[Feat] Implemented the search feature by uploading a image #508
Conversation
WalkthroughAdds a face-search feature: new CONFIDENCE_PERCENT constant; DB helper to fetch all face embeddings; FaceDetector gains a forSearch mode that avoids DB writes; new POST /face-clusters/face-search endpoint performing embedding + cosine similarity matching; legacy facetagging routes removed; frontend adds search slice, file-picker hook, API call, and UI integration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Navbar
participant useFile
participant FrontendAPI
participant Backend
participant FaceDetector
participant DB
note right of Backend #DDEBF7: CONFIDENCE_PERCENT used for matching
User->>Navbar: trigger FaceSearch (upload)
Navbar->>useFile: pickSingleFile()
useFile->>FrontendAPI: POST /face-clusters/face-search { path }
FrontendAPI->>Backend: /face-clusters/face-search { path }
Backend->>FaceDetector: detect_faces(image_id, image_path, forSearch=true)
FaceDetector-->>Backend: embedding(s)
Backend->>DB: get_all_face_embeddings()
DB-->>Backend: stored embeddings + metadata
Backend->>Backend: compute cosine similarities, filter by CONFIDENCE_PERCENT
Backend-->>FrontendAPI: matches (image data with bboxes)
FrontendAPI-->>Navbar: results -> dispatch startSearch + setResults
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 11
🧹 Nitpick comments (15)
backend/app/config/settings.py (1)
7-7: Consider using a more descriptive variable name and add documentation.The constant name
CONFIDENCE_PERCENTcould be more specific to indicate it's used for face matching. Also, the value0.6appears to be a similarity threshold (not a percentage).-CONFIDENCE_PERCENT = 0.6 +# Cosine similarity threshold for face matching (0.0 to 1.0) +# Higher values require closer matches +FACE_SIMILARITY_THRESHOLD = 0.6backend/app/schemas/images.py (2)
36-39: Unused schema definition.The
FaceTaggingResponseschema is defined but doesn't appear to be used by the face-tagging endpoint, which returnsGetAllImagesResponseinstead.Consider removing this unused schema if it's not needed elsewhere:
-class FaceTaggingResponse(BaseModel): - success: bool - message: str - data: dict
6-7: Duplicate AddSingleImageRequest — consolidate definitionAddSingleImageRequest is defined in backend/app/schemas/images.py and backend/app/schemas/test.py; keep a single canonical schema (or rename the test-only class) and update imports to remove the duplicate.
backend/app/routes/face_clusters.py (2)
237-282: Consider performance optimization for large datasets.The current implementation loads all face embeddings into memory and performs linear search. This could be slow for large datasets.
For better scalability, consider:
- Implementing pagination for the results
- Using a vector database (like Faiss, Milvus, or Pinecone) for efficient similarity search
- Building an index for face embeddings to enable faster nearest neighbor search
- Caching frequently accessed embeddings
1-1: Remove unused import.The
loggingmodule is imported but the logger is defined without being used.-import loggingOr use the logger for error handling instead of print statements.
frontend/src/api/api-functions/face_clusters.ts (2)
14-16: Add semicolon for consistencyMissing semicolon after the interface property declaration, inconsistent with the rest of the codebase.
export interface FetchSearchedFacesRequest{ - path: string + path: string; }
44-52: Consider adding error handling and request validationThe function follows the existing pattern well, but consider adding validation for the path parameter to prevent potential issues with empty or invalid paths.
export const fetchSearchedFaces = async ( request: FetchSearchedFacesRequest, ): Promise<APIResponse> => { + if (!request.path || request.path.trim() === '') { + throw new Error('Path is required for face search'); + } const response = await apiClient.post<APIResponse>( faceClustersEndpoints.searchForFaces, request, ); return response.data; };frontend/src/features/searchSlice.ts (1)
27-30: Consider adding error handling stateThe slice handles loading and results but doesn't have an error state. This could leave the UI in an inconsistent state if the search fails.
Add an error field to handle failed searches:
interface SearchState { active: boolean; images: Image[]; loading: boolean; queryImage?: string; + error?: string; } const initialState: SearchState = { active: false, images: [], loading: false, queryImage: undefined, + error: undefined, }; // In the reducers: setResults(state, action: PayloadAction<Image[]>) { state.images = action.payload; state.loading = false; + state.error = undefined; }, + setError(state, action: PayloadAction<string>) { + state.loading = false; + state.error = action.payload; + }, clearSearch(state) { state.active = false; state.images = []; state.loading = false; state.queryImage = undefined; + state.error = undefined; },frontend/src/pages/Home/Home.tsx (1)
43-51: Consider debouncing the loader for search operationsThe loader is shown immediately when search starts, which could cause UI flicker for fast searches. Consider adding a small delay before showing the loader.
useEffect(() => { if (isSearchActive) { if (isSearchLoading) { - dispatch(showLoader('Searching for similar faces...')); + const timer = setTimeout(() => { + dispatch(showLoader('Searching for similar faces...')); + }, 300); // Only show loader if search takes longer than 300ms + return () => clearTimeout(timer); } else { dispatch(hideLoader()); } } }, [isSearchActive, isSearchLoading, dispatch]);frontend/src/hooks/selectFile.ts (3)
41-41: Remove debug logging.Avoid leaking API payloads into the console.
- console.log(res)
6-9: Rename option type for clarity.This hook picks a file, not a folder. Rename
UseFolderPickerOptionstoUseFilePickerOptionsand update usage.-interface UseFolderPickerOptions { +interface UseFilePickerOptions { title?: string; } -export const useFile = ( - options: UseFolderPickerOptions = {}, +export const useFile = ( + options: UseFilePickerOptions = {}, ): UseFilePickerReturn => {Also applies to: 17-21
30-33: Broaden supported image extensions (if backend supports them).Add common formats like webp/tiff/heic/heif.
- extensions: ['png', 'jpg', 'jpeg', 'gif', 'bmp'], + extensions: ['png', 'jpg', 'jpeg', 'gif', 'bmp', 'webp', 'tiff', 'tif', 'heic', 'heif'],frontend/src/components/Navigation/Navbar/Navbar.tsx (3)
26-26: Type the selector with RootState instead of any.Improves safety and editor tooling.
- const searchState = useSelector((state: any) => state.search); + const searchState = useSelector((state: RootState) => state.search);Add import (outside range):
import type { RootState } from '@/app/store';
133-135: Wire the Search icon to open the face-search dialog.Currently it’s a no-op.
- <button className="text-muted-foreground mx-1 hover:text-white"> + <button + className="text-muted-foreground mx-1 hover:text-white" + onClick={() => setIsDialogOpen(true)} + > <Search className="h-4 w-4" /> </button>
112-123: Webcam action is placeholder; add TODO or disable.Prevent confusing UX by disabling or adding a clear TODO until implemented.
Example:
- <Button - onClick={() => {}} - disabled={false} + <Button + onClick={() => {/* TODO: implement webcam capture flow */}} + disabled className="flex h-32 flex-col items-center justify-center gap-2 p-0" variant="outline" >If you want, I can scaffold a minimal webcam capture flow using
MediaDevices.getUserMediaand pipe the blob into the same API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/public/photo.pngis excluded by!**/*.png
📒 Files selected for processing (14)
backend/app/config/settings.py(1 hunks)backend/app/database/faces.py(1 hunks)backend/app/models/FaceDetector.py(2 hunks)backend/app/routes/face_clusters.py(3 hunks)backend/app/routes/facetagging.py(0 hunks)backend/app/schemas/images.py(2 hunks)frontend/src/api/api-functions/face_clusters.ts(2 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/app/store.ts(2 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(4 hunks)frontend/src/features/imageSelectors.ts(1 hunks)frontend/src/features/searchSlice.ts(1 hunks)frontend/src/hooks/selectFile.ts(1 hunks)frontend/src/pages/Home/Home.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- backend/app/routes/facetagging.py
🧰 Additional context used
🧬 Code graph analysis (10)
frontend/src/features/searchSlice.ts (1)
frontend/src/types/Media.ts (1)
Image(1-10)
frontend/src/features/imageSelectors.ts (1)
frontend/src/app/store.ts (1)
RootState(22-22)
frontend/src/hooks/selectFile.ts (3)
frontend/src/types/API.ts (1)
APIResponse(1-8)frontend/src/api/api-functions/face_clusters.ts (1)
fetchSearchedFaces(44-52)frontend/src/types/Media.ts (1)
Image(1-10)
backend/app/models/FaceDetector.py (1)
backend/app/database/faces.py (1)
db_insert_face_embeddings_by_image_id(90-134)
backend/app/schemas/images.py (1)
backend/app/schemas/test.py (1)
AddSingleImageRequest(11-12)
backend/app/database/faces.py (1)
backend/test.py (1)
get_all_face_embeddings(8-32)
frontend/src/pages/Home/Home.tsx (5)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoQuery(80-108)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(5-10)frontend/src/features/loaderSlice.ts (2)
showLoader(17-20)hideLoader(21-24)frontend/src/types/Media.ts (1)
Image(1-10)frontend/src/features/imageSlice.ts (1)
setImages(22-26)
frontend/src/components/Navigation/Navbar/Navbar.tsx (4)
frontend/src/features/onboardingSelectors.ts (2)
selectName(15-15)selectAvatar(13-13)frontend/src/hooks/selectFile.ts (1)
useFile(17-52)frontend/src/features/searchSlice.ts (3)
startSearch(22-26)setResults(27-30)clearSearch(31-36)frontend/src/types/Media.ts (1)
Image(1-10)
backend/app/routes/face_clusters.py (6)
backend/app/database/faces.py (1)
get_all_face_embeddings(136-199)backend/app/models/FaceDetector.py (2)
FaceDetector(11-76)detect_faces(22-64)backend/app/models/FaceNet.py (2)
FaceNet(8-25)get_embedding(16-21)backend/app/schemas/images.py (2)
AddSingleImageRequest(6-7)ErrorResponse(52-55)backend/app/utils/FaceNet.py (1)
FaceNet_util_cosine_similarity(20-23)backend/app/schemas/face_clusters.py (1)
ErrorResponse(23-26)
frontend/src/api/api-functions/face_clusters.ts (3)
frontend/src/types/API.ts (1)
APIResponse(1-8)frontend/src/api/axiosConfig.ts (1)
apiClient(5-12)frontend/src/api/apiEndpoints.ts (1)
faceClustersEndpoints(5-10)
🔇 Additional comments (7)
frontend/src/api/apiEndpoints.ts (1)
7-7: LGTM!The new endpoint follows the existing naming convention and is properly placed within the face clusters endpoints group.
backend/app/models/FaceDetector.py (1)
22-22: Type inconsistency in function signature.The
image_idparameter is typed asstrbut the docstring comment indb_insert_face_embeddings_by_image_idindicates it expects an "Image ID (uuid string)". Good that it's consistent, but ensure all callers are updated.frontend/src/features/imageSelectors.ts (1)
5-10: Add type safety to the search state accessThe selector directly accesses
state.searchwithout proper TypeScript typing. This could lead to runtime errors if the search state structure changes.Import and use the proper RootState type that includes the search slice:
export const selectImages = (state: RootState) => { if (state.search.active) { return state.search.images; } return state.images.images; };Ensure that the RootState type is properly updated with the search slice type definition.
Likely an incorrect or invalid review comment.
frontend/src/app/store.ts (1)
4-4: LGTM! Clean Redux store integrationThe search reducer is properly imported and integrated into the store configuration following the existing pattern.
Also applies to: 18-18
frontend/src/features/searchSlice.ts (1)
1-42: Well-structured Redux slice implementationThe search slice is cleanly implemented with proper TypeScript typing and follows Redux Toolkit best practices. The state structure and actions appropriately handle the search lifecycle.
frontend/src/pages/Home/Home.tsx (2)
65-77: Good UX with helpful empty state messagingThe empty state message provides clear feedback and actionable guidance to users when no results are found.
81-88: Verify ImageCard component accepts new props (imageIndex, className)frontend/src/pages/Home/Home.tsx (lines 81–88) passes imageIndex and className to ImageCard; verification failed because ripgrep returned no searchable files. Ensure ImageCard's props include imageIndex: number and className?: string, or provide the ImageCard file path so I can re-run verification.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend/app/models/FaceDetector.py (1)
20-31: Replace print statements with proper logging.Use the module logger instead of printing to stdout.
- print("FaceDetector initialized with YOLO and FaceNet models.") + logger.info("FaceDetector initialized with YOLO and FaceNet models.") ... - print(f"Failed to load image: {image_path}") + logger.warning("Failed to load image: %s", image_path) ... - print(boxes) - print(f"Detected {len(boxes)} faces in image {image_id}.") + logger.debug("YOLO boxes: %s", boxes) + logger.info("Detected %d faces in image %s.", len(boxes), image_id)Add once near the imports (outside this range):
import logging logger = logging.getLogger(__name__)backend/app/routes/face_clusters.py (2)
42-50: Fix model: bboxes must be a list (multiple faces per image).Current type will fail when passing a list from DB.
class ImageData(BaseModel): @@ - bboxes: BoundingBox + bboxes: List[BoundingBox]
262-275: Fix similarity computation for multiple embeddings (bug) and support multi-face search.You pass a list into FaceNet_util_cosine_similarity and ignore additional faces; this will break or give wrong results.
- process_face = result["processed_faces"][0] - new_embedding = fn.get_embedding(process_face) + # Use embeddings from detector if returned; otherwise compute for all faces + if "embeddings" in result and result["embeddings"]: + search_embeddings = result["embeddings"] + else: + search_embeddings = [fn.get_embedding(pf) for pf in result["processed_faces"]] @@ - else: - for image in images: - max_similarity = 0 - similarity = FaceNet_util_cosine_similarity(new_embedding, image["embeddings"]) - max_similarity = max(max_similarity, similarity) - if max_similarity >= CONFIDENCE_PERCENT: - matches.append(ImageData(id=image["id"], path=image["path"], folder_id=image["folder_id"], thumbnailPath=image["thumbnailPath"], metadata=image["metadata"], isTagged=image["isTagged"], tags=image["tags"], bboxes=image["bbox"])) + else: + for image in images: + stored_embeddings = image.get("embeddings", []) or [] + # best match across all faces in query × all faces in stored image + best = 0.0 + for q in search_embeddings: + for s in stored_embeddings: + sim = FaceNet_util_cosine_similarity(q, s) + if sim > best: + best = sim + if best >= CONFIDENCE_PERCENT: + matches.append( + ImageData( + id=image["id"], + path=image["path"], + folder_id=image["folder_id"], + thumbnailPath=image["thumbnailPath"], + metadata=image["metadata"], + isTagged=image["isTagged"], + tags=image["tags"], + bboxes=image["bbox"], + ) + )
🧹 Nitpick comments (5)
backend/app/models/FaceDetector.py (2)
59-63: Return embeddings to avoid recomputation in the route.You already compute embeddings here; expose them so the endpoint doesn’t re-run FaceNet.
- return { - "ids": f"{class_ids}", - "processed_faces": processed_faces, - "num_faces": len(embeddings), - } + return { + "ids": class_ids, + "processed_faces": processed_faces, + "embeddings": embeddings, + "num_faces": len(embeddings), + }
54-57: Minor: simplify condition and add comment.Parentheses are unnecessary; a short comment helps future readers.
- if (not forSearch and embeddings): + # Persist only when not in search mode and we have embeddings + if not forSearch and embeddings: db_insert_face_embeddings_by_image_id( image_id, embeddings, confidence=confidences, bbox=bboxes )backend/app/routes/face_clusters.py (3)
238-241: Add response_model for OpenAPI and validation.Expose the successful payload model.
-@router.post( - "/face-tagging", - responses={code: {"model": ErrorResponse} for code in [400, 500]}, -) +@router.post( + "/face-tagging", + response_model=GetAllImagesResponse, + responses={code: {"model": ErrorResponse} for code in [400, 500]}, +)
14-15: Remove leftover comment and keep imports clean.Drop the “# Add this import” note.
- db_get_images_by_cluster_id, # Add this import + db_get_images_by_cluster_id,
1-7: Nit: remove unused imports.
numpy.randomisn’t used.-from numpy import random
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/database/faces.py(1 hunks)backend/app/models/FaceDetector.py(2 hunks)backend/app/routes/face_clusters.py(3 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(4 hunks)frontend/src/hooks/selectFile.ts(1 hunks)frontend/src/pages/Home/Home.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/app/database/faces.py
- frontend/src/hooks/selectFile.ts
- frontend/src/components/Navigation/Navbar/Navbar.tsx
- frontend/src/pages/Home/Home.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/routes/face_clusters.py (6)
backend/app/database/faces.py (1)
get_all_face_embeddings(136-203)backend/app/models/FaceDetector.py (3)
FaceDetector(11-75)detect_faces(22-63)close(65-75)backend/app/models/FaceNet.py (2)
FaceNet(8-25)get_embedding(16-21)backend/app/schemas/images.py (2)
AddSingleImageRequest(6-7)ErrorResponse(52-55)backend/app/utils/FaceNet.py (1)
FaceNet_util_cosine_similarity(20-23)backend/app/routes/images.py (2)
ImageData(11-18)GetAllImagesResponse(21-24)
🔇 Additional comments (2)
backend/app/models/FaceDetector.py (1)
22-22: Verify callers use str image_id
- I inspected call sites; face_clusters.py assigns image_id = str(uuid.uuid4()) before calling detect_faces; utils/images.py calls detect_faces(image_id, image_path) — please confirm image_id in that code path is a str (if it's sourced from DB or request it may be int/UUID object).
backend/app/routes/face_clusters.py (1)
281-283: Good: resources are closed reliably.try/finally cleanup on FaceDetector and FaceNet looks correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
frontend/src/components/Navigation/Navbar/Navbar.tsx (2)
19-19: Resolved: Image type is now imported.Thanks for addressing the missing type import flagged earlier.
34-42: Guard API result, remove unsafe cast, and always clear loading (wrap in try/catch/finally).Current flow assumes success and double-casts to Image[], which can push undefined/invalid data into state and leave loading/dialog states inconsistent on failures.
Apply this diff:
- const handlePickFile = async () => { - const file = await pickSingleFile(); - if (file) { - dispatch(startSearch(file.path)); - const result = file.result.data as unknown as Image[]; - dispatch(setResults(result)); - setIsDialogOpen(false); - } - }; + const handlePickFile = async () => { + try { + const file = await pickSingleFile(); + if (!file) return; + dispatch(startSearch(file.path)); + const apiResult = file.result; + const payload = + apiResult && apiResult.success && Array.isArray(apiResult.data) + ? (apiResult.data as Image[]) + : []; + dispatch(setResults(payload)); + } catch { + // Ensure loading is cleared on unexpected errors + dispatch(setResults([])); + } finally { + // Always close dialog + setIsDialogOpen(false); + } + };backend/app/routes/face_clusters.py (3)
41-50: bboxes should be a list, not a single BoundingBox (runtime validation error).DB returns multiple boxes per image; model declares a single box. Make it a list with a safe default.
-class ImageData(BaseModel): +class ImageData(BaseModel): id: str path: str folder_id: str thumbnailPath: str metadata: str isTagged: bool tags: Optional[List[str]] = None - bboxes: BoundingBox + bboxes: List[BoundingBox] = []If you prefer avoiding mutable defaults, switch to Pydantic Field:
-from pydantic import BaseModel +from pydantic import BaseModel, Field @@ - bboxes: List[BoundingBox] = [] + bboxes: List[BoundingBox] = Field(default_factory=list)
240-251: Security: do not accept arbitrary filesystem paths; accept UploadFile instead. Also clean up temp files.
- Switch to UploadFile, persist to a temp file for cv2.imread, and unlink in finally.
- Drop unused AddSingleImageRequest import.
-from fastapi import APIRouter, HTTPException, status +from fastapi import APIRouter, HTTPException, status, UploadFile, File-from app.schemas.images import AddSingleImageRequest -from app.utils.FaceNet import FaceNet_util_cosine_similarity +from app.utils.FaceNet import FaceNet_util_cosine_similarity-def face_tagging(payload: AddSingleImageRequest): - image_path = payload.path - if not os.path.isfile(image_path): +def face_tagging(file: UploadFile = File(...)): + import tempfile, shutil + # Persist upload to a temp file for OpenCV + suffix = os.path.splitext(getattr(file, "filename", "") or "")[1] + tmp = tempfile.NamedTemporaryFile(delete=False, suffix=suffix) + try: + shutil.copyfileobj(file.file, tmp) + tmp.flush(); tmp.close() + image_path = tmp.name + except Exception as e: + try: + tmp.close() + os.unlink(tmp.name) + except Exception: + pass + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, error="Invalid upload", message=str(e) + ).model_dump(), + ) + if not os.path.isfile(image_path): raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=ErrorResponse( success=False, error="Invalid file path", message="The provided path is not a valid file", ).model_dump(), ) @@ finally: fd.close() fn.close() + # Cleanup temp file + try: + os.unlink(image_path) + except Exception: + passAlso applies to: 280-282, 8-8, 30-31
261-274: Fix similarity logic: handle multiple search faces and per-image embedding lists.Current code passes a list into cosine_similarity and ignores additional faces in the search image. Compute max similarity across all pairs.
- process_face = result["processed_faces"][0] - new_embedding = fn.get_embedding(process_face) + # Compute embeddings for all detected faces in the search image + search_embeddings = [fn.get_embedding(pf) for pf in result["processed_faces"]] @@ - else: - for image in images: - max_similarity = 0 - similarity = FaceNet_util_cosine_similarity(new_embedding, image["embeddings"]) - max_similarity = max(max_similarity, similarity) - if max_similarity >= CONFIDENCE_PERCENT: - matches.append(ImageData(id=image["id"], path=image["path"], folder_id=image["folder_id"], thumbnailPath=image["thumbnailPath"], metadata=image["metadata"], isTagged=image["isTagged"], tags=image["tags"], bboxes=image["bbox"])) + else: + for image in images: + stored_embeddings = image.get("embeddings") or [] + max_similarity = -1.0 + for se in search_embeddings: + for te in stored_embeddings: + sim = FaceNet_util_cosine_similarity(se, te) + if sim > max_similarity: + max_similarity = sim + if max_similarity >= CONFIDENCE_PERCENT: + matches.append( + ImageData( + id=image["id"], + path=image["path"], + folder_id=image["folder_id"], + thumbnailPath=image["thumbnailPath"], + metadata=image["metadata"], + isTagged=image["isTagged"], + tags=image["tags"], + bboxes=image["bbox"], + ) + )
🧹 Nitpick comments (9)
frontend/src/components/Navigation/Navbar/Navbar.tsx (5)
26-29: Use typed, narrow selectors to avoid unnecessary re-renders and remove any.Select only what you need and type RootState.
Apply this diff:
- // Move all useSelector calls to the top level - const searchState = useSelector((state: any) => state.search); - const isSearchActive = searchState.active; - const queryImage = searchState.queryImage; + // Move all useSelector calls to the top level + const isSearchActive = useSelector((s: RootState) => s.search.active); + const queryImage = useSelector((s: RootState) => s.search.queryImage); + const isLoading = useSelector((s: RootState) => s.search.loading);Add missing import (outside range):
import type { RootState } from '@/app/store';
100-111: Disable actions while searching to prevent duplicate requests.Tie button disabled state to the slice’s loading flag.
- <Button - onClick={handlePickFile} - disabled={false} + <Button + onClick={handlePickFile} + disabled={isLoading} className="flex h-32 flex-col items-center justify-center gap-2 p-0" variant="outline" > @@ - <Button - onClick={() => {}} - disabled={false} + <Button + onClick={() => {}} + disabled={isLoading} className="flex h-32 flex-col items-center justify-center gap-2 p-0" variant="outline" >Also applies to: 113-124
61-64: A11y + minor cleanup: improve alt text, add aria-label, and drop unreachable fallback.- <img - src={convertFileSrc(queryImage) || 'photo.png'} - alt="Query" - className="h-7 w-7 rounded object-cover" - /> + <img + src={convertFileSrc(queryImage)} + alt="Query face preview" + className="h-7 w-7 rounded object-cover" + /> @@ - <button + <button + aria-label="Clear face search" onClick={() => dispatch(clearSearch())} className="absolute -top-1 -right-1 flex h-3 w-3 items-center justify-center rounded-full bg-red-600 text-[10px] leading-none text-white" >Also applies to: 66-73
31-31: Consider exposing selection before fetch in useFile to show loading earlier.Right now, startSearch runs after the hook returns, i.e., after the network call completes. If you adjust useFile to return the selected path immediately (and the fetch as a separate promise or callback), you can dispatch startSearch(path) before the request to reflect loading in UI during the fetch.
Happy to propose a small change in frontend/src/hooks/selectFile.ts to support this pattern (e.g., return { pickPath, fetchResults } or an option like { autoFetch: false }).
134-136: No-op quick search button. Intentional?This button has no handler. Wire it to focus the input or open the face search dialog, or remove it.
backend/app/routes/face_clusters.py (4)
236-239: Add explicit response_model for OpenAPI and validation.@router.post( "/face-tagging", + response_model=GetAllImagesResponse, responses={code: {"model": ErrorResponse} for code in [400, 500]}, )
9-14: Remove leftover “# Add this import” inline note.- db_get_images_by_cluster_id, # Add this import + db_get_images_by_cluster_id,
252-263: Avoid double inference: reuse embeddings from FaceDetector.FaceDetector already computes embeddings; returning them from detect_faces would avoid re-running FaceNet here.
Follow-up: extend FaceDetector.detect_faces to include embeddings in the returned dict when forSearch=True and then drop the separate FaceNet instantiation here.
52-56: Deduplicate response models.ImageData/GetAllImagesResponse are also defined in backend/app/routes/images.py; prefer a single shared schema to avoid drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/routes/face_clusters.py(3 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/routes/face_clusters.py (7)
backend/app/database/faces.py (1)
get_all_face_embeddings(136-203)backend/app/models/FaceDetector.py (3)
FaceDetector(11-75)detect_faces(22-63)close(65-75)backend/app/models/FaceNet.py (2)
FaceNet(8-25)get_embedding(16-21)backend/app/schemas/images.py (2)
AddSingleImageRequest(6-7)ErrorResponse(52-55)backend/app/utils/FaceNet.py (1)
FaceNet_util_cosine_similarity(20-23)backend/app/routes/images.py (2)
ImageData(11-18)GetAllImagesResponse(21-24)backend/app/schemas/face_clusters.py (1)
ErrorResponse(23-26)
frontend/src/components/Navigation/Navbar/Navbar.tsx (4)
frontend/src/features/onboardingSelectors.ts (2)
selectName(15-15)selectAvatar(13-13)frontend/src/hooks/selectFile.ts (1)
useFile(16-51)frontend/src/features/searchSlice.ts (3)
startSearch(22-26)setResults(27-30)clearSearch(31-36)frontend/src/types/Media.ts (1)
Image(1-10)
🔇 Additional comments (2)
backend/app/routes/face_clusters.py (2)
280-282: LGTM: resources are closed in a finally block.
272-272: No change required — CONFIDENCE_PERCENT uses 0–1 scalebackend/app/config/settings.py defines CONFIDENCE_PERCENT = 0.6, so comparing cosine similarity (range -1..1) against it is correct.
frontend/src/pages/Home/Home.tsx
Outdated
| <h1 className="mb-6 text-2xl font-bold">{title}</h1> | ||
|
|
||
| {/* Show message when search is active but no results */} | ||
| {isSearchActive && searchResults.length === 0 && !isSearchLoading && ( | ||
| <div className="flex items-center justify-center py-12"> | ||
| <div className="text-center"> | ||
| <p className="text-muted-foreground mb-2 text-lg"> | ||
| No matching faces found | ||
| </p> | ||
| <p className="text-muted-foreground text-sm"> | ||
| Try uploading a different image or clear the search to see all | ||
| images | ||
| </p> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Use the InfoDialog using Redux state: frontend/src/features/infoDialogSlice.ts
There was a problem hiding this comment.
In this file, move the <Dialog> component and its related functions to another file so that the file remains modular
frontend/src/features/searchSlice.ts
Outdated
| interface SearchState { | ||
| active: boolean; | ||
| images: Image[]; | ||
| loading: boolean; |
There was a problem hiding this comment.
Remove the loading state from here.
In one of my comments, I have mentioned about usePictoMutation, use the "isLoading" returned from it to show loaders and decide the logic.
frontend/src/api/apiEndpoints.ts
Outdated
|
|
||
| export const faceClustersEndpoints = { | ||
| getAllClusters: '/face-clusters/', | ||
| searchForFaces: '/face-clusters/face-tagging', |
There was a problem hiding this comment.
rename this endpoint to face-clusters/face-search
| if (selected && typeof selected === 'string') { | ||
| const res: APIResponse = await fetchSearchedFaces({ | ||
| path: selected, | ||
| } as FetchSearchedFacesRequest); | ||
| console.log(res) | ||
| return { path: selected, result: res }; | ||
| } | ||
| return null; | ||
| } catch (error) { | ||
| console.error('Error picking File:', error); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Use the standardized way to send a POST request, i.e., use the usePictoMutation function. Refer to how to use it from other files.
Also, move the API logic to the handlePickFile function. Or when you create a separate component for the Face upload dialog(as requested in the previous comments), call the usePictoMutation there. This hook should only handle the file selection.
| if (state.search.active) { | ||
| return state.search.images; | ||
| } |
There was a problem hiding this comment.
Remove this part. selectImages should always return all the images. I mentioned something opposite in the issue, but now I realize that a separate state for search(Like you have done) is necessary.
| @router.post( | ||
| "/face-tagging", | ||
| responses={code: {"model": ErrorResponse} for code in [400, 500]}, | ||
| ) |
There was a problem hiding this comment.
When you created this endpoint, the openapi.json file must have updated automatically. Please commit that file so that our API docs remain updated.
|
@tushar1977, I have added the requested changes. Kindly update your PR with these changes. Also, resolve any linting errors that may arise. |
sure will be done by tomorrow |
rahulharpal1603
force-pushed
the
main
branch
from
f589c41 to
00d7a55
Compare
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 (2)
backend/app/utils/microservice.py (2)
78-85: Avoid piping long‑lived process stdout/stderr; risk of deadlock when buffers fill.The microservice will continuously log; with PIPEs and no readers, it can block. Send output to DEVNULL or inherit parent stdio.
Apply this diff:
- process = subprocess.Popen( - [str(sync_executable)], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=True, - cwd=str(sync_dir), # Set working directory to sync service directory - ) + process = subprocess.Popen( + [str(sync_executable)], + cwd=str(sync_dir), # Set working directory to sync service directory + stdout=subprocess.DEVNULL, + stderr=subprocess.STDOUT, + text=True, + )
259-263: Same stdout/stderr PIPE issue in dev; also prefer cwd over chdir to avoid global side‑effects.Use cwd in Popen and drop global os.chdir changes; prevent output pipe buildup.
Apply this diff:
- process = subprocess.Popen( - cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True - ) + process = subprocess.Popen( + cmd, + cwd=str(service_path), + stdout=subprocess.DEVNULL, + stderr=subprocess.STDOUT, + text=True, + )Outside selected lines, you can remove the directory juggling:
- original_cwd = os.getcwd() - os.chdir(service_path) ... - os.chdir(original_cwd)
🧹 Nitpick comments (12)
sync-microservice/.gitignore (2)
143-143: Ignore both directory and potential file variantIf tooling ever creates a file named .sync-env (not a dir), ignore it too.
Apply this diff:
.sync-env/ +.sync-env
139-140: Use recursive glob to ignore dist/ but keep README**Verified sync-microservice/dist/README.md is tracked.
File: sync-microservice/.gitignore (lines 139-140)
-dist/* +dist/** !dist/README.mdfrontend/src/components/Navigation/Navbar/Navbar.tsx (1)
15-17: Use typed selector and avoidanytype.Instead of casting the state to
any, use proper typing for the Redux state selector to maintain type safety.- const searchState = useSelector((state: any) => state.search); - const isSearchActive = searchState.active; - const queryImage = searchState.queryImage; + const isSearchActive = useSelector((state: RootState) => state.search.active); + const queryImage = useSelector((state: RootState) => state.search.queryImage);Add the missing
RootStateimport:import type { RootState } from '@/app/store';frontend/src/components/Dialog/FaceSearchDialog.tsx (1)
103-114: Webcam functionality is not implemented.The webcam button has a placeholder
onClick={() => {}}handler. Consider either implementing the functionality or disabling the button with appropriate visual feedback.<Button onClick={() => {}} - disabled={false} + disabled={true} className="flex h-32 flex-col items-center justify-center gap-2 p-0" variant="outline" > <Camera className="text-muted-foreground mb-1 h-8 w-8" /> <span className="text-sm font-medium">Use Webcam</span> - <span className="text-muted-foreground text-center text-xs"> - Capture with camera - </span> + <span className="text-muted-foreground text-center text-xs"> + Coming Soon + </span> </Button>Would you like me to help implement the webcam functionality or create an issue to track this feature?
frontend/src/components/Media/MediaView.tsx (1)
38-38: Remove debug console.log statement.The console.log statement should be removed from production code.
}, [images, currentViewIndex]); - console.log(currentViewIndex);docs/backend/backend_python/openapi.json (3)
19-37: Health endpoint summary is stale (“Root”).Rename summary to reflect /health and keep naming consistent across docs.
- "summary": "Root", + "summary": "Health Check",
1038-1045: Endpoint naming mismatch: “Face Tagging” vs “face-search”.Align summary/operationId with the new feature to avoid confusion.
- "summary": "Face Tagging", - "operationId": "face_tagging_face_clusters_face_search_post", + "summary": "Face Search", + "operationId": "face_search_face_clusters_face_search_post",
1291-1303: Clarify AddSingleImageRequest purpose or avoid coupling it to uploads.If the endpoint moves to multipart uploads, this JSON “path” model may be unused here. Either:
- Keep it (if other routes use it) and add a description (e.g., “Server-accessible image path”), or
- Rename to ImagePathRequest for clarity.
Can you confirm where AddSingleImageRequest is referenced in backend routes besides face-search?
backend/app/utils/microservice.py (4)
55-55: Verify PyInstaller path resolution; consider robust fallback.Using parent.parent assumes a specific bundle layout. If packaging varies, sync-microservice may not be found. Consider checking multiple candidates (sys._MEIPASS, parent, parent.parent) and pick the first that contains sync-microservice.
Example approach (outside selected lines):
# Prefer MEIPASS when present, then exe_dir, then its parent exe_dir = Path(sys.executable).parent candidates = [ Path(getattr(sys, "_MEIPASS", "")), exe_dir, exe_dir.parent, ] for base in candidates: if base and (base / "sync-microservice").exists(): app_dir = base break else: app_dir = exe_dir # fallback
60-62: Naming/docstring mismatch (PictoPy_Sync vs sync-microservice).Docstring still mentions “PictoPy_Sync folder” while code looks for sync-microservice. Update for consistency.
Outside selected lines, update the docstring:
- The sync microservice executable should be in the PictoPy_Sync folder. + The sync microservice executable should reside in the 'sync-microservice' directory.
254-256: Bind host on non‑Windows for parity and clarity.fastapi dev defaults can vary; be explicit to ensure it binds only to localhost like Windows.
Apply this diff:
- cmd = [str(python_executable), "-m", "fastapi", "dev", "--port", "8001"] + cmd = [ + str(python_executable), "-m", "fastapi", "dev", + "--host", host, + "--port", port, + ]
236-238: Remove debug print; use logger instead.Stray print in library code is noisy.
Replace (outside selected lines):
- print(python_executable) + logger.debug("Using venv Python: %s", python_executable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/ISSUE_TEMPLATE/bug.yml(0 hunks).github/ISSUE_TEMPLATE/feature.yml(0 hunks).github/release-drafter-config.yml(2 hunks).github/workflows/update-project-structure.yml(1 hunks).gitignore(2 hunks)backend/app/database/faces.py(1 hunks)backend/app/routes/face_clusters.py(3 hunks)backend/app/utils/microservice.py(2 hunks)docs/backend/backend_python/openapi.json(3 hunks)frontend/src/api/api-functions/face_clusters.ts(2 hunks)frontend/src/api/apiEndpoints.ts(2 hunks)frontend/src/components/Dialog/FaceSearchDialog.tsx(1 hunks)frontend/src/components/Media/MediaView.tsx(2 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(1 hunks)frontend/src/features/imageSelectors.ts(1 hunks)frontend/src/features/searchSlice.ts(1 hunks)frontend/src/hooks/selectFile.ts(1 hunks)frontend/src/pages/AITagging/AITagging.tsx(1 hunks)frontend/src/pages/Home/Home.tsx(2 hunks)frontend/src/types/Media.ts(1 hunks)sync-microservice/.gitignore(1 hunks)
💤 Files with no reviewable changes (2)
- .github/ISSUE_TEMPLATE/feature.yml
- .github/ISSUE_TEMPLATE/bug.yml
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- .github/workflows/update-project-structure.yml
- .github/release-drafter-config.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/app/routes/face_clusters.py
- frontend/src/features/imageSelectors.ts
- backend/app/database/faces.py
- frontend/src/api/api-functions/face_clusters.ts
- frontend/src/pages/Home/Home.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/src/features/searchSlice.ts (1)
frontend/src/types/Media.ts (1)
Image(1-10)
frontend/src/components/Dialog/FaceSearchDialog.tsx (7)
frontend/src/hooks/selectFile.ts (1)
useFile(11-39)frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/face_clusters.ts (1)
fetchSearchedFaces(44-52)frontend/src/types/Media.ts (1)
Image(1-10)frontend/src/features/loaderSlice.ts (2)
hideLoader(21-24)showLoader(17-20)frontend/src/features/searchSlice.ts (3)
setResults(24-26)clearSearch(27-31)startSearch(20-23)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)
frontend/src/components/Media/MediaView.tsx (2)
frontend/src/types/Media.ts (1)
MediaViewProps(25-29)frontend/src/features/imageSelectors.ts (1)
selectCurrentViewIndex(8-9)
frontend/src/pages/AITagging/AITagging.tsx (1)
frontend/src/components/Media/MediaView.tsx (1)
MediaView(26-192)
frontend/src/components/Navigation/Navbar/Navbar.tsx (3)
frontend/src/features/onboardingSelectors.ts (2)
selectName(15-15)selectAvatar(13-13)frontend/src/features/searchSlice.ts (1)
clearSearch(27-31)frontend/src/components/Dialog/FaceSearchDialog.tsx (1)
FaceSearchDialog(20-124)
🔇 Additional comments (13)
frontend/src/features/searchSlice.ts (1)
1-37: LGTM! Well-structured Redux Toolkit slice implementation.This search slice follows the Redux Toolkit standard approach for writing Redux logic and correctly uses
createSlicefor state management. The implementation is clean with proper TypeScript interfaces, clear reducer logic, and appropriate exports.frontend/src/hooks/selectFile.ts (1)
1-40: LGTM! Clean file picker implementation with proper error handling.The previous comments requested using
usePictoMutationand moving API logic elsewhere, but those concerns have been addressed since the API logic was moved toFaceSearchDialog.tsxcomponent, making this a focused, single-purpose hook for file selection only.frontend/src/types/Media.ts (1)
27-27: LGTM! Proper type definition update aligns with MediaView component changes.The addition of the required
images: Image[]prop toMediaViewPropscorrectly reflects the component's new API signature and supports the centralized data flow pattern implemented across the application.frontend/src/pages/AITagging/AITagging.tsx (1)
68-68: LGTM! Correct prop passing to updated MediaView component.The change to pass
images={taggedImages}aligns with the MediaView component's new prop-based data flow, replacing the previous reliance on Redux selectors.frontend/src/api/apiEndpoints.ts (2)
7-7: LGTM! Endpoint follows the requested naming convention.The previous comment requested renaming this endpoint to
face-clusters/face-search, and this has been correctly implemented as shown.
21-28: Incorrect — these endpoints are used in the codebaseuserPreferencesEndpoints is referenced in frontend/src/api/api-functions/user_preferences.ts; healthEndpoints is referenced in frontend/src/api/api-functions/health.ts — do not remove.
Likely an incorrect or invalid review comment.
frontend/src/components/Navigation/Navbar/Navbar.tsx (2)
34-50: LGTM! Query image display with clear action is well implemented.The implementation correctly displays the selected query image with a clear button that dispatches the
clearSearch()action. The conditional rendering and styling are appropriate.
59-61: LGTM! Clean integration with FaceSearchDialog component.The previous comment requested moving the dialog component to a separate file, and this has been correctly implemented by using the
FaceSearchDialogcomponent.frontend/src/components/Dialog/FaceSearchDialog.tsx (2)
26-56: LGTM! Proper mutation handling with comprehensive error management.The
usePictoMutationimplementation correctly handles success/error states, manages loading states, and provides appropriate user feedback through info dialogs. The logic flow properly differentiates between successful results with data versus no matches found.
58-66: LGTM! Clean file selection workflow with proper state management.The
handlePickFilefunction correctly integrates file selection with Redux state management - closing the dialog, getting the file path, dispatching search actions, and triggering the mutation.frontend/src/components/Media/MediaView.tsx (1)
32-37: LGTM! Proper prop-based data flow with memoization.The transition from Redux selectors to prop-based data access is well implemented. The
useMemousage forcurrentImagecomputation is appropriate for performance optimization.docs/backend/backend_python/openapi.json (1)
1046-1054: Docs claim multipart 'upload' but implementation uses JSON path — reconcileFrontend posts JSON ({ path }) — see frontend/src/components/Dialog/FaceSearchDialog.tsx and frontend/src/api/api-functions/face_clusters.ts (endpoint: frontend/src/api/apiEndpoints.ts → /face-clusters/face-search); backend contains no UploadFile/File references. Either update docs/openapi to document application/json { path } or change FE+backend to accept multipart/form-data file uploads (do not change only the OpenAPI without aligning implementation).
Likely an incorrect or invalid review comment.
backend/app/utils/microservice.py (1)
254-256: fastapi CLI and uvicorn present in requirements
backend/requirements.txt contains fastapi-cli==0.0.3 and uvicorn==0.30.1; sync-microservice/requirements.txt contains fastapi-cli==0.0.8 and uvicorn==0.35.0 — the "python -m fastapi dev" CLI should be available in the venv.
| "responses": { | ||
| "200": { | ||
| "description": "Successful Response", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": {} | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
200 response has an empty schema. Define a typed response for client generation.
Provide a concrete response (e.g., success wrapper with results array including confidence/bbox).
- "200": {
- "description": "Successful Response",
- "content": {
- "application/json": {
- "schema": {}
- }
- }
- },
+ "200": {
+ "description": "Successful Response",
+ "content": {
+ "application/json": {
+ "schema": {
+ "$ref": "#/components/schemas/SearchFacesResponse"
+ }
+ }
+ }
+ },Add these schemas under components.schemas (outside this hunk):
{
"SearchFacesData": {
"type": "object",
"properties": {
"results": {
"type": "array",
"items": { "$ref": "#/components/schemas/ImageInCluster" },
"title": "Results"
},
"total_results": { "type": "integer", "title": "Total Results" }
},
"required": ["results", "total_results"],
"title": "SearchFacesData"
},
"SearchFacesResponse": {
"type": "object",
"properties": {
"success": { "type": "boolean", "title": "Success" },
"message": {
"anyOf": [{ "type": "string" }, { "type": "null" }],
"title": "Message"
},
"error": {
"anyOf": [{ "type": "string" }, { "type": "null" }],
"title": "Error"
},
"data": {
"anyOf": [{ "$ref": "#/components/schemas/SearchFacesData" }, { "type": "null" }]
}
},
"required": ["success"],
"title": "SearchFacesResponse"
}
}Also applies to: 1074-1094
🤖 Prompt for AI Agents
In docs/backend/backend_python/openapi.json around lines 1055-1063 (and also
1074-1094), the 200 responses use an empty schema; replace those empty schemas
with a $ref to a concrete response schema (e.g.,
"#/components/schemas/SearchFacesResponse") and add the two new schemas under
components.schemas (SearchFacesData and SearchFacesResponse) as provided in the
review comment so the response is typed for client generation.
2 participants
Overview
This PR implements a new API route to search a tagged face and deleting the old file of
routes/facetagging.pyIssue ID :- #502
Changes
routes/face-cluster.pySummary by CodeRabbit
New Features
Documentation
Chores