UI Based Multi-Person Face Image Search with Ranked Results#1304
Open
rohan-pandeyy wants to merge 6 commits into
Open
UI Based Multi-Person Face Image Search with Ranked Results#1304rohan-pandeyy wants to merge 6 commits into
rohan-pandeyy wants to merge 6 commits into
Conversation
added new schema, route and db functions
…w dialog component and corresponding API endpoints.
github-actions
Bot
added
GSoC 2026
backend
enhancement
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR implements a complete multi-person face search feature. Users can open a modal from Face Collections to select multiple face clusters, choose "any" or "all" match mode, and retrieve photos ranked by co-appearance count. The backend queries the database with configurable match logic; the frontend maps results into Redux and displays them in a ranked or chronological gallery. ChangesMulti-Person Face Search Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
backend/app/routes/face_clusters.py (1)
370-378: ⚡ Quick winMove match_mode validation to the Pydantic schema.
The runtime validation here duplicates what could be expressed declaratively in the schema. Update
MultiPersonSearchRequest.match_modeto useLiteral["match_any", "match_all"]instead ofstr, which will:
- Provide compile-time type safety
- Auto-generate accurate OpenAPI enum documentation
- Eliminate the need for this runtime check
# In backend/app/schemas/face_clusters.py from typing import Literal class MultiPersonSearchRequest(BaseModel): cluster_ids: List[str] match_mode: Literal["match_any", "match_all"] = "match_any"Then remove lines 370-378.
🤖 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 `@backend/app/routes/face_clusters.py` around lines 370 - 378, Update the MultiPersonSearchRequest Pydantic model to declare match_mode as a Literal by changing its type from str to Literal["match_any", "match_all"] and set the default to "match_any" (symbol: MultiPersonSearchRequest.match_mode in backend/app/schemas/face_clusters.py); after that remove the runtime validation block that checks body.match_mode in the face_clusters route (the if body.match_mode not in (...) raise HTTPException block) so the schema enforces allowed values and OpenAPI enum docs are generated automatically.backend/app/database/face_clusters.py (1)
368-381: 💤 Low valueConsider simplifying the GROUP BY clause.
Since
i.idis the primary key of the images table, the other selected columns (i.path,i.thumbnailPath,i.metadata) are functionally dependent on it. In SQLite 3.7.11+ (when compiled withSQLITE_ENABLE_STAT4), you can group by just the primary key:- GROUP BY i.id, i.path, i.thumbnailPath, i.metadata + GROUP BY i.idThis improves clarity and may offer minor performance benefits by reducing the grouping work.
🤖 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 `@backend/app/database/face_clusters.py` around lines 368 - 381, The GROUP BY clause in the base_sql string in face_clusters.py is overly verbose; change the GROUP BY from "GROUP BY i.id, i.path, i.thumbnailPath, i.metadata" to just "GROUP BY i.id" (since i.id is the primary key and the other columns are functionally dependent), keeping the rest of the query (SELECT, HAVING placeholder, ORDER BY) unchanged and ensuring the variable name base_sql and any surrounding code that injects {having} still work as before.frontend/src/components/Dialog/MultiPersonSearchDialog.tsx (2)
68-75: ⚡ Quick winRemove
anyfrom search-result mapping.
images.map((img: any) ... ) as Image[]bypasses type safety on a new API contract. Type the payload and map without a blanket cast.Suggested fix
+type MultiPersonSearchApiImage = { + id: string; + path: string; + thumbnailPath?: string; + metadata: Image['metadata']; +}; - - const mappedImages = images.map((img: any) => ({ + const mappedImages: Image[] = images.map((img: MultiPersonSearchApiImage) => ({ id: img.id, path: img.path, thumbnailPath: img.thumbnailPath || '', metadata: img.metadata, folder_id: '', isTagged: true, - })) as Image[]; + }));As per coding guidelines: "Avoid 'any', use explicit types."
🤖 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 `@frontend/src/components/Dialog/MultiPersonSearchDialog.tsx` around lines 68 - 75, The mapping uses a blanket any and then casts to Image[]; define an explicit input type (e.g., interface ApiImage { id: string; path: string; thumbnailPath?: string; metadata: any; folder_id?: string; /* other fields from API */ }) and replace images.map((img: any) => ...) with images.map((img: ApiImage) => ({ id: img.id, path: img.path, thumbnailPath: img.thumbnailPath ?? '', metadata: img.metadata, folder_id: img.folder_id ?? '', isTagged: true })) and let TypeScript infer the result or assert the final array as Image[] only if necessary, removing the use of any on images and the blanket cast.
37-260: 🏗️ Heavy liftAdd automated tests for the multi-person search dialog flow.
This introduces critical behavior (selection, Any/All mode, success/no-match/error branches) without corresponding automated coverage in this PR slice.
As per coding guidelines: "Ensure that test code is automated, comprehensive, and follows testing best practices" and "Verify that all critical functionality is covered by tests."
🤖 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 `@frontend/src/components/Dialog/MultiPersonSearchDialog.tsx` around lines 37 - 260, Add automated tests for the MultiPersonSearchDialog component covering selection toggling, match mode switching (Any/All), Clear button behavior, Search button enabling/disabling, and all mutate outcome branches: success with images (ensure dispatch(setImages) called, onSearchActivated invoked with selected names and matchMode, and onOpenChange(false) called), success with no images (ensure showInfoDialog "No Matches Found"), and error branch (ensure showInfoDialog "Search Failed"). Mock the usePictoMutation/mutate function to assert it receives { cluster_ids: [...selectedIds], match_mode: matchMode } and mock dispatch to verify showLoader/hideLoader and setImages/showInfoDialog calls; use React Testing Library to render MultiPersonSearchDialog, simulate clicks on cluster buttons and the Switch, and assert UI state (badge/count/checkbox) and side effects for each scenario.frontend/src/components/FaceCollections.tsx (1)
81-81: ⚡ Quick winReplace
anywithClusterin the cluster list mapping.This keeps rendering logic type-safe and prevents accidental property drift.
Suggested fix
- {clusters.map((cluster: any) => ( + {clusters.map((cluster: Cluster) => (As per coding guidelines: "Avoid 'any', use explicit types."
🤖 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 `@frontend/src/components/FaceCollections.tsx` at line 81, Replace the usage of the unsafe any in the JSX mapping by typing the iterator as Cluster: change the mapping callback signature where clusters.map is used to accept (cluster: Cluster) instead of (cluster: any); ensure the Cluster interface/type is imported or defined in this module (or the parent props type) and update any surrounding prop/type annotations (e.g., the component props or clusters declaration) to use Cluster so the render logic is type-safe.
🤖 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 `@frontend/src/pages/AITagging/AITagging.tsx`:
- Around line 89-93: The badges use key={name} which can collide for identical
display names; update the mapping in AITagging.tsx to provide stable unique keys
(e.g. use the underlying cluster/person id or a composite key) instead of the
raw name: locate the usage of searchState.peopleNames and the Badge component
and change the iteration to access a unique identifier (such as cluster.id or
person.id) or fall back to a composite key like `${name}-${index}` so each Badge
has a stable, unique key for React reconciliation.
---
Nitpick comments:
In `@backend/app/database/face_clusters.py`:
- Around line 368-381: The GROUP BY clause in the base_sql string in
face_clusters.py is overly verbose; change the GROUP BY from "GROUP BY i.id,
i.path, i.thumbnailPath, i.metadata" to just "GROUP BY i.id" (since i.id is the
primary key and the other columns are functionally dependent), keeping the rest
of the query (SELECT, HAVING placeholder, ORDER BY) unchanged and ensuring the
variable name base_sql and any surrounding code that injects {having} still work
as before.
In `@backend/app/routes/face_clusters.py`:
- Around line 370-378: Update the MultiPersonSearchRequest Pydantic model to
declare match_mode as a Literal by changing its type from str to
Literal["match_any", "match_all"] and set the default to "match_any" (symbol:
MultiPersonSearchRequest.match_mode in backend/app/schemas/face_clusters.py);
after that remove the runtime validation block that checks body.match_mode in
the face_clusters route (the if body.match_mode not in (...) raise HTTPException
block) so the schema enforces allowed values and OpenAPI enum docs are generated
automatically.
In `@frontend/src/components/Dialog/MultiPersonSearchDialog.tsx`:
- Around line 68-75: The mapping uses a blanket any and then casts to Image[];
define an explicit input type (e.g., interface ApiImage { id: string; path:
string; thumbnailPath?: string; metadata: any; folder_id?: string; /* other
fields from API */ }) and replace images.map((img: any) => ...) with
images.map((img: ApiImage) => ({ id: img.id, path: img.path, thumbnailPath:
img.thumbnailPath ?? '', metadata: img.metadata, folder_id: img.folder_id ?? '',
isTagged: true })) and let TypeScript infer the result or assert the final array
as Image[] only if necessary, removing the use of any on images and the blanket
cast.
- Around line 37-260: Add automated tests for the MultiPersonSearchDialog
component covering selection toggling, match mode switching (Any/All), Clear
button behavior, Search button enabling/disabling, and all mutate outcome
branches: success with images (ensure dispatch(setImages) called,
onSearchActivated invoked with selected names and matchMode, and
onOpenChange(false) called), success with no images (ensure showInfoDialog "No
Matches Found"), and error branch (ensure showInfoDialog "Search Failed"). Mock
the usePictoMutation/mutate function to assert it receives { cluster_ids:
[...selectedIds], match_mode: matchMode } and mock dispatch to verify
showLoader/hideLoader and setImages/showInfoDialog calls; use React Testing
Library to render MultiPersonSearchDialog, simulate clicks on cluster buttons
and the Switch, and assert UI state (badge/count/checkbox) and side effects for
each scenario.
In `@frontend/src/components/FaceCollections.tsx`:
- Line 81: Replace the usage of the unsafe any in the JSX mapping by typing the
iterator as Cluster: change the mapping callback signature where clusters.map is
used to accept (cluster: Cluster) instead of (cluster: any); ensure the Cluster
interface/type is imported or defined in this module (or the parent props type)
and update any surrounding prop/type annotations (e.g., the component props or
clusters declaration) to use Cluster so the render logic is type-safe.
🪄 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: adb2295c-8dfd-48e2-9498-2bf94f8c5734
📒 Files selected for processing (13)
backend/app/database/face_clusters.pybackend/app/database/images.pybackend/app/routes/face_clusters.pybackend/app/routes/images.pybackend/app/schemas/face_clusters.pydocs/backend/backend_python/openapi.jsonfrontend/src/api/api-functions/face_clusters.tsfrontend/src/api/apiEndpoints.tsfrontend/src/components/Dialog/MultiPersonSearchDialog.tsxfrontend/src/components/FaceCollections.tsxfrontend/src/components/PersonAvatar.tsxfrontend/src/pages/AITagging/AITagging.tsxfrontend/src/utils/personUtils.ts
Comment on lines
+89
to
+93
| {searchState.peopleNames.map((name) => ( | ||
| <Badge key={name} variant="secondary" className="text-xs"> | ||
| {name} | ||
| </Badge> | ||
| ))} |
Contributor
There was a problem hiding this comment.
Use unique keys for selected-person badges.
key={name} can collide when two selected clusters share the same display name, causing React reconciliation issues.
Suggested fix
- {searchState.peopleNames.map((name) => (
- <Badge key={name} variant="secondary" className="text-xs">
+ {searchState.peopleNames.map((name, index) => (
+ <Badge key={`${name}-${index}`} variant="secondary" className="text-xs">
{name}
</Badge>
))}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {searchState.peopleNames.map((name) => ( | |
| <Badge key={name} variant="secondary" className="text-xs"> | |
| {name} | |
| </Badge> | |
| ))} | |
| {searchState.peopleNames.map((name, index) => ( | |
| <Badge key={`${name}-${index}`} variant="secondary" className="text-xs"> | |
| {name} | |
| </Badge> | |
| ))} |
🤖 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 `@frontend/src/pages/AITagging/AITagging.tsx` around lines 89 - 93, The badges
use key={name} which can collide for identical display names; update the mapping
in AITagging.tsx to provide stable unique keys (e.g. use the underlying
cluster/person id or a composite key) instead of the raw name: locate the usage
of searchState.peopleNames and the Badge component and change the iteration to
access a unique identifier (such as cluster.id or person.id) or fall back to a
composite key like `${name}-${index}` so each Badge has a stable, unique key for
React reconciliation.
Contributor
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
…feat/multi-person-face-search-&-ranking # Conflicts: # backend/app/routes/face_clusters.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/backend/backend_python/openapi.json (1)
1171-1242:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign multi-search OpenAPI contract with backend validation and actual responses.
MultiPersonSearchRequestcurrently permits anymatch_modestring and an emptycluster_idsarray, but the route rejects those (match_modemust bematch_any|match_all,cluster_idscannot be empty). Also, this operation documents404though the route logic shown returns 200/400/500 only. This contract drift can mislead client generation and integration error handling.Suggested OpenAPI adjustments
"MultiPersonSearchRequest": { "properties": { "cluster_ids": { "items": { "type": "string" }, "type": "array", + "minItems": 1, "title": "Cluster Ids" }, "match_mode": { - "type": "string", + "type": "string", + "enum": ["match_any", "match_all"], "title": "Match Mode", "default": "match_any" } }, "type": "object", "required": ["cluster_ids"], "title": "MultiPersonSearchRequest" }"/face-clusters/multi-search": { "post": { ... "responses": { "200": { ... }, "400": { ... }, - "404": { ... }, "500": { ... }, "422": { ... } } } }As per coding guidelines, "Confirm that the code meets the project's requirements and objectives"; the OpenAPI contract should match
backend/app/routes/face_clusters.py(Lines 357-416) andbackend/app/schemas/face_clusters.py(Lines 89-112).Also applies to: 2826-2846
🤖 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 `@docs/backend/backend_python/openapi.json` around lines 1171 - 1242, Update the OpenAPI operation search_images_by_multiple_faces_face_clusters_multi_search_post so the request schema MultiPersonSearchRequest enforces match_mode as an enum ["match_any","match_all"] (not free-form string) and requires cluster_ids with minItems: 1 (disallow empty arrays), ensure responses reflect actual route behavior by removing the 404 response and keeping 200, 400, 500, 422 (refer to MultiPersonSearchResponse and app__schemas__face_clusters__ErrorResponse for response schema refs); adjust the components/schemas for MultiPersonSearchRequest accordingly to match the validation in the route and schema definitions.Source: Coding guidelines
🧹 Nitpick comments (1)
frontend/src/components/Media/RankedGallery.tsx (1)
34-37: ⚡ Quick winMake gallery header responsive and deduplicate with
ChronologicalGallery.The same title row markup appears in both galleries and can compress/overflow on narrow screens (
justify-betweensingle row). Use a shared header component (or shared class pattern) with wrap/stack behavior.♻️ Suggested change
- <div className="mt-6 mb-6 flex items-center justify-between"> + <div className="mt-6 mb-6 flex flex-col gap-2 sm:flex-row sm:items-center sm:justify-between"> <h1 className="text-2xl font-bold">{title}</h1> {titleRight && <div>{titleRight}</div>} </div>As per coding guidelines, “Ensure responsive design principles are followed” and “Look for code duplication.”
🤖 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 `@frontend/src/components/Media/RankedGallery.tsx` around lines 34 - 37, The title row in RankedGallery (the <h1> title and optional titleRight) is duplicated in ChronologicalGallery and currently uses a single-row layout that can overflow; extract this into a shared GalleryHeader component (or a shared CSS class) that accepts props title and titleRight, replace the duplicated markup in RankedGallery and ChronologicalGallery with that component, and make the header responsive by using wrapping/stacking styles (e.g., apply flex with flex-wrap/justify-between plus gap and/or a mobile breakpoint to switch to vertical stacking) so the title and titleRight never overlap on narrow screens.Source: Coding guidelines
🤖 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 `@docs/backend/backend_python/openapi.json`:
- Line 1553: The OpenAPI operation currently lists duplicate tags ("tags":
["Shutdown", "Shutdown"]); remove the redundant entry so the tags array contains
a single "Shutdown" value (e.g., change to "tags": ["Shutdown"]) to prevent
duplicate grouping in generated docs and keep metadata clean.
---
Outside diff comments:
In `@docs/backend/backend_python/openapi.json`:
- Around line 1171-1242: Update the OpenAPI operation
search_images_by_multiple_faces_face_clusters_multi_search_post so the request
schema MultiPersonSearchRequest enforces match_mode as an enum
["match_any","match_all"] (not free-form string) and requires cluster_ids with
minItems: 1 (disallow empty arrays), ensure responses reflect actual route
behavior by removing the 404 response and keeping 200, 400, 500, 422 (refer to
MultiPersonSearchResponse and app__schemas__face_clusters__ErrorResponse for
response schema refs); adjust the components/schemas for
MultiPersonSearchRequest accordingly to match the validation in the route and
schema definitions.
---
Nitpick comments:
In `@frontend/src/components/Media/RankedGallery.tsx`:
- Around line 34-37: The title row in RankedGallery (the <h1> title and optional
titleRight) is duplicated in ChronologicalGallery and currently uses a
single-row layout that can overflow; extract this into a shared GalleryHeader
component (or a shared CSS class) that accepts props title and titleRight,
replace the duplicated markup in RankedGallery and ChronologicalGallery with
that component, and make the header responsive by using wrapping/stacking styles
(e.g., apply flex with flex-wrap/justify-between plus gap and/or a mobile
breakpoint to switch to vertical stacking) so the title and titleRight never
overlap on narrow screens.
🪄 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: 099ab4e3-2017-4f30-a78a-ab493f43226d
📒 Files selected for processing (9)
backend/app/routes/face_clusters.pybackend/app/schemas/face_clusters.pydocs/backend/backend_python/openapi.jsonfrontend/src/api/api-functions/face_clusters.tsfrontend/src/components/FaceCollections.tsxfrontend/src/components/GallerySortDropdown.tsxfrontend/src/components/Media/ChronologicalGallery.tsxfrontend/src/components/Media/RankedGallery.tsxfrontend/src/pages/AITagging/AITagging.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/app/schemas/face_clusters.py
- frontend/src/api/api-functions/face_clusters.ts
- backend/app/routes/face_clusters.py
- frontend/src/components/FaceCollections.tsx
- frontend/src/pages/AITagging/AITagging.tsx
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/backend/backend_python/openapi.json (1)
1171-1242:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign multi-search OpenAPI contract with backend validation and actual responses.
MultiPersonSearchRequestcurrently permits anymatch_modestring and an emptycluster_idsarray, but the route rejects those (match_modemust bematch_any|match_all,cluster_idscannot be empty). Also, this operation documents404though the route logic shown returns 200/400/500 only. This contract drift can mislead client generation and integration error handling.Suggested OpenAPI adjustments
"MultiPersonSearchRequest": { "properties": { "cluster_ids": { "items": { "type": "string" }, "type": "array", + "minItems": 1, "title": "Cluster Ids" }, "match_mode": { - "type": "string", + "type": "string", + "enum": ["match_any", "match_all"], "title": "Match Mode", "default": "match_any" } }, "type": "object", "required": ["cluster_ids"], "title": "MultiPersonSearchRequest" }"/face-clusters/multi-search": { "post": { ... "responses": { "200": { ... }, "400": { ... }, - "404": { ... }, "500": { ... }, "422": { ... } } } }As per coding guidelines, "Confirm that the code meets the project's requirements and objectives"; the OpenAPI contract should match
backend/app/routes/face_clusters.py(Lines 357-416) andbackend/app/schemas/face_clusters.py(Lines 89-112).Also applies to: 2826-2846
🤖 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 `@docs/backend/backend_python/openapi.json` around lines 1171 - 1242, Update the OpenAPI operation search_images_by_multiple_faces_face_clusters_multi_search_post so the request schema MultiPersonSearchRequest enforces match_mode as an enum ["match_any","match_all"] (not free-form string) and requires cluster_ids with minItems: 1 (disallow empty arrays), ensure responses reflect actual route behavior by removing the 404 response and keeping 200, 400, 500, 422 (refer to MultiPersonSearchResponse and app__schemas__face_clusters__ErrorResponse for response schema refs); adjust the components/schemas for MultiPersonSearchRequest accordingly to match the validation in the route and schema definitions.Source: Coding guidelines
🧹 Nitpick comments (1)
frontend/src/components/Media/RankedGallery.tsx (1)
34-37: ⚡ Quick winMake gallery header responsive and deduplicate with
ChronologicalGallery.The same title row markup appears in both galleries and can compress/overflow on narrow screens (
justify-betweensingle row). Use a shared header component (or shared class pattern) with wrap/stack behavior.♻️ Suggested change
- <div className="mt-6 mb-6 flex items-center justify-between"> + <div className="mt-6 mb-6 flex flex-col gap-2 sm:flex-row sm:items-center sm:justify-between"> <h1 className="text-2xl font-bold">{title}</h1> {titleRight && <div>{titleRight}</div>} </div>As per coding guidelines, “Ensure responsive design principles are followed” and “Look for code duplication.”
🤖 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 `@frontend/src/components/Media/RankedGallery.tsx` around lines 34 - 37, The title row in RankedGallery (the <h1> title and optional titleRight) is duplicated in ChronologicalGallery and currently uses a single-row layout that can overflow; extract this into a shared GalleryHeader component (or a shared CSS class) that accepts props title and titleRight, replace the duplicated markup in RankedGallery and ChronologicalGallery with that component, and make the header responsive by using wrapping/stacking styles (e.g., apply flex with flex-wrap/justify-between plus gap and/or a mobile breakpoint to switch to vertical stacking) so the title and titleRight never overlap on narrow screens.Source: Coding guidelines
🤖 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 `@docs/backend/backend_python/openapi.json`:
- Line 1553: The OpenAPI operation currently lists duplicate tags ("tags":
["Shutdown", "Shutdown"]); remove the redundant entry so the tags array contains
a single "Shutdown" value (e.g., change to "tags": ["Shutdown"]) to prevent
duplicate grouping in generated docs and keep metadata clean.
---
Outside diff comments:
In `@docs/backend/backend_python/openapi.json`:
- Around line 1171-1242: Update the OpenAPI operation
search_images_by_multiple_faces_face_clusters_multi_search_post so the request
schema MultiPersonSearchRequest enforces match_mode as an enum
["match_any","match_all"] (not free-form string) and requires cluster_ids with
minItems: 1 (disallow empty arrays), ensure responses reflect actual route
behavior by removing the 404 response and keeping 200, 400, 500, 422 (refer to
MultiPersonSearchResponse and app__schemas__face_clusters__ErrorResponse for
response schema refs); adjust the components/schemas for
MultiPersonSearchRequest accordingly to match the validation in the route and
schema definitions.
---
Nitpick comments:
In `@frontend/src/components/Media/RankedGallery.tsx`:
- Around line 34-37: The title row in RankedGallery (the <h1> title and optional
titleRight) is duplicated in ChronologicalGallery and currently uses a
single-row layout that can overflow; extract this into a shared GalleryHeader
component (or a shared CSS class) that accepts props title and titleRight,
replace the duplicated markup in RankedGallery and ChronologicalGallery with
that component, and make the header responsive by using wrapping/stacking styles
(e.g., apply flex with flex-wrap/justify-between plus gap and/or a mobile
breakpoint to switch to vertical stacking) so the title and titleRight never
overlap on narrow screens.
🪄 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: 099ab4e3-2017-4f30-a78a-ab493f43226d
📒 Files selected for processing (9)
backend/app/routes/face_clusters.pybackend/app/schemas/face_clusters.pydocs/backend/backend_python/openapi.jsonfrontend/src/api/api-functions/face_clusters.tsfrontend/src/components/FaceCollections.tsxfrontend/src/components/GallerySortDropdown.tsxfrontend/src/components/Media/ChronologicalGallery.tsxfrontend/src/components/Media/RankedGallery.tsxfrontend/src/pages/AITagging/AITagging.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/app/schemas/face_clusters.py
- frontend/src/api/api-functions/face_clusters.ts
- backend/app/routes/face_clusters.py
- frontend/src/components/FaceCollections.tsx
- frontend/src/pages/AITagging/AITagging.tsx
🛑 Comments failed to post (1)
docs/backend/backend_python/openapi.json (1)
1553-1553:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicated
Shutdowntag entry.
"tags": ["Shutdown", "Shutdown"]is redundant and can create noisy grouping in generated API docs.As per coding guidelines, "Point out redundant obvious comments that do not add clarity to the code"; similarly, redundant OpenAPI metadata should be removed for cleaner documentation.
🤖 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 `@docs/backend/backend_python/openapi.json` at line 1553, The OpenAPI operation currently lists duplicate tags ("tags": ["Shutdown", "Shutdown"]); remove the redundant entry so the tags array contains a single "Shutdown" value (e.g., change to "tags": ["Shutdown"]) to prevent duplicate grouping in generated docs and keep metadata clean.Source: Coding guidelines
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.
Closes #1302
This pull request introduces a new "multi-person search" feature to the backend and API, allowing users to search for images containing multiple specified face cluster identities, with results ranked by the number of matches. It includes the necessary backend database logic, API endpoints, request/response schemas, OpenAPI documentation, and frontend API integration. Additionally, there are minor improvements to error handling and code formatting.
Multi-person search feature:
db_get_images_by_face_clustersinface_clusters.pyto retrieve images containing any or all of a list of face cluster IDs, with results ranked by the number of matching clusters per image.MultiPersonSearchRequest,MultiPersonSearchImage,MultiPersonSearchData,MultiPersonSearchResponse) inface_clusters.pyschemas to support the multi-person search API./face-clusters/multi-searchPOST endpoint inface_clusters.py, implementing validation and response logic for multi-person search requests.openapi.json) to document the new endpoint and its request/response schemas. [1] [2]face_clusters.tsand registered the endpoint inapiEndpoints.ts. [1] [2]Other improvements:
Screenshots/Recordings:
Additional Notes:
AI Usage Disclosure:
I have used the following AI models and tools: Claude, Gemini
Checklist
Summary by CodeRabbit