feat: WebCam feature #583
feat: WebCam feature #583
Conversation
This reverts commit 52a8a57. good
…og after selecting a file
|
|
WalkthroughReplaces single-input face-tagging with a dual-input POST /face-clusters/face-search (path or base64 via query Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Frontend UI (Dialog / Webcam)
participant FE as Frontend API
participant BE as Backend / face_tagging
participant Search as perform_face_search
rect rgb(220,235,255)
Note over User,UI: Upload / Path flow (input_type=path)
User->>UI: Select file (path)
UI->>FE: fetchSearchedFaces(filePath) -- ?input_type=path
FE->>BE: POST /face-clusters/face-search?input_type=path (body: {path})
BE->>Search: perform_face_search(image_path)
Search-->>BE: GetAllImagesResponse
BE-->>FE: Results
FE-->>UI: Render matches
end
rect rgb(235,255,235)
Note over User,UI: Webcam / Base64 flow (input_type=base64)
User->>UI: Capture photo → base64
UI->>FE: fetchSearchedFacesBase64({base64_data}) -- ?input_type=base64
FE->>BE: POST /face-clusters/face-search?input_type=base64 (body: {base64_data})
BE->>BE: validate & decode base64 → write temp file
BE->>Search: perform_face_search(temp_path)
Search-->>BE: GetAllImagesResponse
BE->>BE: cleanup temp file
BE-->>FE: Results
FE-->>UI: Render matches
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
frontend/src-tauri/src/main.rs (2)
17-23: Consider removing or documenting the debug print statement.The setup closure resolves the backend resource path but only prints it without further use. If this is temporary debugging code, consider removing it. If it's intentional for logging during startup, consider using proper logging instead of
println!.
4-4: Remove unusedservicesmodule
Theservicesmodule only definesget_server_pathbut isn’t invoked by Tauri or referenced anywhere else. Removemod services;inmain.rs,pub mod services;inlib.rs, and deletefrontend/src-tauri/src/services/.backend/app/routes/face_clusters.py (1)
264-274: Add input validation for base64 data.The endpoint should validate:
- The base64 data can be successfully decoded
- The decoded data represents a valid image format
- The data size is within acceptable limits to prevent DoS attacks via extremely large payloads
Consider wrapping the base64 decode and file operations in a try-except block to return a proper error response instead of letting exceptions propagate.
+ MAX_IMAGE_SIZE = 10 * 1024 * 1024 # 10MB limit + image_path = None try: + # Validate and decode base64 + try: + image_bytes = base64.b64decode(base64_data.split(",")[-1]) + except Exception: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, + error="Invalid base64 data", + message="The provided base64 data could not be decoded.", + ).model_dump(), + ) + + # Validate size + if len(image_bytes) > MAX_IMAGE_SIZE: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, + error="Image too large", + message=f"Image size exceeds maximum allowed size of {MAX_IMAGE_SIZE} bytes.", + ).model_dump(), + ) + - image_bytes = base64.b64decode(base64_data.split(",")[-1]) image_id = str(uuid.uuid4())[:8]docs/backend/backend_python/openapi.json (1)
2058-2058: Confirm cluster metadata flexibility is intentionalOpenAPI’s
additionalProperties: trueforImageInCluster.metadatamatches Python’sOptional[Dict[str, Any]]and its dynamic use in face_clusters, YOLO, ONNX, etc. If you require stricter contracts, define or reuse a dedicated metadata schema (e.g.MetadataModel) and document expected keys.backend/app/utils/faceSearch.py (2)
45-46: Consider reusing detector instances to avoid initialization overhead.Creating new
FaceDetectorandFaceNetinstances on every search call incurs significant overhead—loading YOLO and FaceNet models from disk, allocating ONNX sessions, etc. For a webcam-based search feature where users may perform multiple consecutive searches, this becomes a performance bottleneck.Refactor to use shared instances or dependency injection:
# Option 1: Module-level singleton (if thread-safe) _detector_instance = None _facenet_instance = None def get_detector_instances(): global _detector_instance, _facenet_instance if _detector_instance is None: _detector_instance = FaceDetector() if _facenet_instance is None: _facenet_instance = FaceNet(DEFAULT_FACENET_MODEL) return _detector_instance, _facenet_instance def perform_face_search(image_path: str) -> GetAllImagesResponse: fd, fn = get_detector_instances() # Remove the finally block that closes instances ...# Option 2: Pass instances as parameters (better for testing) def perform_face_search( image_path: str, fd: FaceDetector, fn: FaceNet ) -> GetAllImagesResponse: # No initialization or cleanup needed ...If you choose Option 2, update the calling route to manage the lifecycle of these instances (e.g., create once per worker process or use FastAPI dependencies).
50-50: Remove unused UUID generation.The
image_idvariable (line 50) is generated but only used for logging withindetect_faces. Since this is a search operation and the ID isn't persisted or returned, generating a UUID adds unnecessary overhead.Consider removing it or passing a constant placeholder:
- image_id = str(uuid.uuid4()) - result = fd.detect_faces(image_id, image_path, forSearch=True) + result = fd.detect_faces("search_temp", image_path, forSearch=True)Or if logging requires unique IDs, document why it's necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
backend/app/routes/face_clusters.py(3 hunks)backend/app/schemas/images.py(1 hunks)backend/app/utils/faceSearch.py(1 hunks)docs/backend/backend_python/openapi.json(3 hunks)frontend/package.json(1 hunks)frontend/src-tauri/src/main.rs(1 hunks)frontend/src-tauri/tauri.conf.json(1 hunks)frontend/src/api/api-functions/face_clusters.ts(2 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/components/Dialog/FaceSearchDialog.tsx(2 hunks)frontend/src/components/WebCam/WebCamComponent.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
backend/app/routes/face_clusters.py (3)
backend/app/schemas/images.py (3)
AddSingleBase64ImageRequest(10-11)AddSingleImageRequest(6-7)ErrorResponse(57-60)backend/app/utils/faceSearch.py (1)
perform_face_search(35-95)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-11)
backend/app/utils/faceSearch.py (4)
backend/app/models/FaceDetector.py (2)
FaceDetector(11-75)detect_faces(22-63)backend/app/models/FaceNet.py (2)
FaceNet(8-25)get_embedding(16-21)backend/app/utils/FaceNet.py (1)
FaceNet_util_cosine_similarity(20-23)backend/app/routes/face_clusters.py (3)
BoundingBox(30-34)ImageData(37-45)GetAllImagesResponse(48-51)
frontend/src/components/WebCam/WebCamComponent.tsx (6)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/face_clusters.ts (1)
fetchSearchedFacesBase64(58-67)frontend/src/types/Media.ts (1)
Image(13-22)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/Dialog/FaceSearchDialog.tsx (4)
frontend/src/constants/routes.ts (1)
ROUTES(1-11)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)frontend/src/features/searchSlice.ts (1)
startSearch(20-23)frontend/src/features/loaderSlice.ts (1)
showLoader(17-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (windows-latest)
🔇 Additional comments (11)
frontend/package.json (1)
64-64: LGTM! Dependency addition aligns with webcam feature.The
react-webcamdependency at version ^7.2.0 is correctly added to support the new webcam capture functionality introduced in this PR.backend/app/schemas/images.py (1)
10-12: LGTM! Schema follows established patterns.The new
AddSingleBase64ImageRequestmodel correctly mirrors the structure ofAddSingleImageRequestand appropriately supports the base64 image search endpoint.frontend/src/api/apiEndpoints.ts (1)
8-8: LGTM! Endpoint mapping is correct.The new endpoint path correctly aligns with the backend route
/face-clusters/face-search-base64.docs/backend/backend_python/openapi.json (2)
1097-1135: LGTM! New endpoint definition is complete and correct.The
/face-clusters/face-search-base64endpoint definition properly specifies the request schema, response codes, and follows the existing API patterns.
1330-1342: LGTM! Schema definition matches backend model.The
AddSingleBase64ImageRequestschema correctly defines the requiredbase64_datafield and aligns with the backend Pydantic model.frontend/src/api/api-functions/face_clusters.ts (1)
18-20: LGTM! Interface correctly defines the base64 request structure.The
FetchSearchedFacesBase64Requestinterface appropriately mirrorsFetchSearchedFacesRequestwith the base64-specific field.frontend/src/components/WebCam/WebCamComponent.tsx (1)
29-166: LGTM! Webcam component implementation is well-structured.The component correctly implements the capture-review-search flow with appropriate state management, error handling via info dialogs, and proper cleanup of state on close. The integration with Redux slices and the mutation hook follows established patterns in the codebase.
backend/app/routes/face_clusters.py (1)
247-247: LGTM! Refactored to use centralized search logic.The refactoring to delegate to
perform_face_searchimproves code maintainability by centralizing the face detection and search logic.frontend/src/components/Dialog/FaceSearchDialog.tsx (1)
92-153: LGTM! Dialog redesign enhances user experience.The refactored dialog structure with dual action buttons (Upload Photo and Use Webcam) provides clear user choices. The webcam permission handling with appropriate error feedback and the integration of
WebcamComponentare well implemented.backend/app/utils/faceSearch.py (2)
60-61: Clarify behavior: only the first detected face is processed.The code only processes
result["processed_faces"][0], discarding all other detected faces in the image. This may be intentional for the webcam use case (search for one person at a time), but it's not documented and could be surprising behavior.If this is intentional, add a comment explaining why:
- process_face = result["processed_faces"][0] - new_embedding = fn.get_embedding(process_face) + # For webcam search, we only match the first detected face + # Users should ensure only one face is visible in the frame + process_face = result["processed_faces"][0] + new_embedding = fn.get_embedding(process_face)If multiple faces should be supported, consider processing all faces and returning the union of matches, or providing an option to specify which face to search for.
71-85: Remove this review comment. The concern is incorrect.The code at
faceSearch.py:72is correct.image["embeddings"]is a JSON-parsed list (returned fromget_all_face_embeddingsat line 180 offaces.py), andFaceNet_util_cosine_similarityhandles lists without issue—numpy operations automatically accept list inputs.The real underlying issue is in
get_all_face_embeddings(line 171), which only stores the first face per image (if image_id not in images_dict:), causing silent data loss when multiple faces exist in the same image. However, this is a separate concern from the faceSearch.py code structure.Likely an incorrect or invalid review comment.
| class BoundingBox(BaseModel): | ||
| x: float | ||
| y: float | ||
| width: float | ||
| height: float | ||
|
|
||
|
|
||
| class ImageData(BaseModel): | ||
| id: str | ||
| path: str | ||
| folder_id: str | ||
| thumbnailPath: str | ||
| metadata: Dict[str, Any] | ||
| isTagged: bool | ||
| tags: Optional[List[str]] = None | ||
| bboxes: BoundingBox | ||
|
|
||
|
|
||
| class GetAllImagesResponse(BaseModel): | ||
| success: bool | ||
| message: str | ||
| data: List[ImageData] |
There was a problem hiding this comment.
Eliminate duplicate model definitions.
The BoundingBox, ImageData, and GetAllImagesResponse models are already defined identically in backend/app/routes/face_clusters.py (lines 29-50). This code duplication violates DRY principles and creates a maintenance burden—future changes must be synchronized across both locations.
Move these models to a shared location (e.g., backend/app/schemas/face_search.py or backend/app/models/schemas.py) and import them in both files:
-from typing import Optional, List, Dict, Any
-from pydantic import BaseModel
+from app.schemas.face_search import BoundingBox, ImageData, GetAllImagesResponse
from app.config.settings import CONFIDENCE_PERCENT, DEFAULT_FACENET_MODEL
from app.database.faces import get_all_face_embeddings
from app.models.FaceDetector import FaceDetector
from app.models.FaceNet import FaceNet
from app.utils.FaceNet import FaceNet_util_cosine_similarity
-
-
-class BoundingBox(BaseModel):
- x: float
- y: float
- width: float
- height: float
-
-
-class ImageData(BaseModel):
- id: str
- path: str
- folder_id: str
- thumbnailPath: str
- metadata: Dict[str, Any]
- isTagged: bool
- tags: Optional[List[str]] = None
- bboxes: BoundingBox
-
-
-class GetAllImagesResponse(BaseModel):
- success: bool
- message: str
- data: List[ImageData]Then update backend/app/routes/face_clusters.py to import from the same shared location.
Committable suggestion skipped: line range outside the PR's diff.
| try: | ||
| matches = [] | ||
| image_id = str(uuid.uuid4()) | ||
|
|
||
| result = fd.detect_faces(image_id, image_path, forSearch=True) | ||
| if not result or result["num_faces"] == 0: | ||
| return GetAllImagesResponse( | ||
| success=True, | ||
| message="No faces detected in the image.", | ||
| data=[], | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for image loading and detection failures.
The function does not handle potential failures such as:
- Invalid or corrupted image files
- Model inference errors
- File system access issues
Without explicit error handling, exceptions will propagate to the caller with potentially unclear error messages.
Wrap the detection logic in a try-except block:
try:
matches = []
image_id = str(uuid.uuid4())
- result = fd.detect_faces(image_id, image_path, forSearch=True)
+ try:
+ result = fd.detect_faces(image_id, image_path, forSearch=True)
+ except Exception as e:
+ return GetAllImagesResponse(
+ success=False,
+ message=f"Failed to process image: {str(e)}",
+ data=[],
+ )
+
if not result or result["num_faces"] == 0:
return GetAllImagesResponse(
success=True,
message="No faces detected in the image.",
data=[],
)Also consider validating that image_path exists before passing it to detect_faces:
import os
if not os.path.exists(image_path):
return GetAllImagesResponse(
success=False,
message="Image file not found.",
data=[],
)🤖 Prompt for AI Agents
In backend/app/utils/faceSearch.py around lines 48 to 58, the detection block
lacks validation and error handling for missing/corrupt files and runtime
errors; first check os.path.exists(image_path) and return a GetAllImagesResponse
with success=False and a clear "Image file not found." message if missing, then
wrap the call to fd.detect_faces(...) and subsequent processing in a try/except
that catches broad exceptions (or more specific ones if known), logs the
exception detail, and returns a GetAllImagesResponse with success=False, a
descriptive error message and empty data so callers receive a clear failure
response instead of an unhandled exception.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/components/Dialog/FaceSearchDialog.tsx (3)
62-79: Consider enhancing error handling.The previous resource leak and UX flow issues have been correctly addressed. However, consider these improvements:
- Error logging: The caught error (line 69) is not logged, which could make debugging permission issues harder.
- Error guidance: The error message doesn't guide users on how to enable camera permissions if they were previously denied.
Apply this diff to add error logging:
} catch (error) { + console.error('Webcam access error:', error); dispatch( showInfoDialog({ title: 'Webcam Not Supported',
95-95: Remove redundantonClickhandler.The
DialogTriggercomponent automatically handles opening the dialog when clicked, so the explicitonClick={() => setIsDialogOpen(true)}is redundant and can be removed.Apply this diff:
<Button - onClick={() => setIsDialogOpen(true)} variant="ghost"
116-116: Remove redundantdisabled={false}props.The
disabled={false}props on lines 116 and 129 are redundant since buttons are enabled by default.Apply this diff:
<Button onClick={handlePickFile} - disabled={false} className="flex h-32 flex-col items-center justify-center gap-2 p-0"<Button onClick={handleWebCam} - disabled={false} className="flex h-32 flex-col items-center justify-center gap-2 p-0"Also applies to: 129-129
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/utils/faceSearch.py(1 hunks)frontend/src/components/Dialog/FaceSearchDialog.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/utils/faceSearch.py
🔇 Additional comments (3)
frontend/src/components/Dialog/FaceSearchDialog.tsx (3)
22-22: LGTM! Clean integration of webcam functionality.The WebcamComponent import and
showCamerastate are properly integrated to support the new webcam capture feature.Also applies to: 26-26
80-89: LGTM! Proper dialog close timing.Closing the dialog after file selection (line 84) ensures the dialog remains open if the user cancels the file picker, which is the correct UX flow.
148-151: Consider conditional rendering for WebcamComponent.The WebcamComponent is always rendered regardless of the
showCamerastate. If this component has expensive initialization (e.g., loading camera libraries, setting up media streams), consider conditional rendering for better performance.Run the following script to check if WebcamComponent has expensive initialization:
If the component does perform expensive operations on mount, apply this diff:
-<WebcamComponent - isOpen={showCamera} - onClose={() => setShowCamera(false)} -/> +{showCamera && ( + <WebcamComponent + isOpen={showCamera} + onClose={() => setShowCamera(false)} + /> +)}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
frontend/src/components/WebCam/WebCamComponent.tsx (4)
35-66: Simplify by removing redundant handleClose() call.The
handleClose()is called in two places: line 82 (before search starts) and line 41 (after search completes). Since the dialog closes immediately when search begins (line 82), the second call inonSuccessis redundant.Apply this diff:
onSuccess: (data) => { const result = data?.data as Image[]; dispatch(hideLoader()); - handleClose(); if (result && result.length > 0) {
81-96: Add error handling for camera permissions and initialization.There's no error handling if the user denies camera access or if the camera fails to initialize. This could leave users confused with a black screen or no feedback.
Consider adding an error handler to the
Webcamcomponent and displaying appropriate feedback:const [cameraError, setCameraError] = useState<string | null>(null); const handleUserMediaError = (error: string | DOMException) => { console.error('Camera error:', error); const message = typeof error === 'string' ? error : error.name === 'NotAllowedError' ? 'Camera access was denied. Please allow camera access and try again.' : 'Failed to access camera. Please check your device settings.'; dispatch( showInfoDialog({ title: 'Camera Error', message, variant: 'error', }), ); setCameraError(message); };Then pass it to the
Webcamcomponent:<Webcam audio={false} height={500} ref={webcamRef} screenshotFormat="image/jpeg" width={500} videoConstraints={videoConstraints} className="rounded-lg border" onUserMediaError={handleUserMediaError} />
119-167: Simplify conditional rendering logic.The condition
showCamera && !capturedImageUrlon line 119 includes a redundant check. WhencapturedImageUrlis set (line 71),showCamerais immediately set tofalse(line 72), so!capturedImageUrlis alwaystruewhenshowCameraistrue.Apply this diff to simplify:
- {showCamera && !capturedImageUrl ? ( + {showCamera ? ( <div className="flex flex-col items-center gap-4">Alternatively, you could simplify the entire conditional structure by just using
capturedImageUrlas the single source of truth:{!capturedImageUrl ? ( // Show camera <div className="flex flex-col items-center gap-4"> <Webcam ... /> <Button onClick={capture}>Capture Photo</Button> </div> ) : ( // Show captured image <div className="flex flex-col items-center gap-4"> <img src={capturedImageUrl} ... /> <div className="flex gap-3">...</div> </div> )}This eliminates the need for the
showCamerastate entirely.
121-129: Add accessibility labels for screen readers.The webcam video stream and captured image lack proper ARIA labels, which could make the component less accessible to users with screen readers.
Apply these diffs:
<Webcam audio={false} ref={webcamRef} screenshotFormat="image/jpeg" videoConstraints={videoConstraints} className="rounded-lg border w-full max-w-md" + aria-label="Webcam video stream for face capture" /><img src={capturedImageUrl} alt="Captured" + role="img" + aria-label="Captured photo for face search" className="max-h-96 rounded-lg border" />Also applies to: 136-140
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/public/photo.jpegis excluded by!**/*.jpeg
📒 Files selected for processing (2)
frontend/src/components/Navigation/Navbar/Navbar.tsx(1 hunks)frontend/src/components/WebCam/WebCamComponent.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/Navigation/Navbar/Navbar.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/WebCam/WebCamComponent.tsx (7)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/face_clusters.ts (1)
fetchSearchedFacesBase64(58-66)frontend/src/types/Media.ts (1)
Image(13-22)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/ui/dialog.tsx (5)
Dialog(131-131)DialogContent(133-133)DialogHeader(136-136)DialogTitle(139-139)DialogDescription(134-134)
| const capture = useCallback(() => { | ||
| if (webcamRef.current) { | ||
| const imageSrc = webcamRef.current.getScreenshot(); | ||
| setCapturedImageUrl(imageSrc); | ||
| setShowCamera(false); | ||
| } | ||
| }, [webcamRef]); |
There was a problem hiding this comment.
Add null check for getScreenshot() result.
The getScreenshot() method can return null if the webcam stream isn't ready or if there's an error. This would cause capturedImageUrl to be set to null, and the user would see nothing after clicking "Capture Photo".
Apply this diff:
const capture = useCallback(() => {
if (webcamRef.current) {
const imageSrc = webcamRef.current.getScreenshot();
- setCapturedImageUrl(imageSrc);
- setShowCamera(false);
+ if (imageSrc) {
+ setCapturedImageUrl(imageSrc);
+ setShowCamera(false);
+ } else {
+ dispatch(
+ showInfoDialog({
+ title: 'Capture Failed',
+ message: 'Unable to capture image. Please try again.',
+ variant: 'error',
+ }),
+ );
+ }
}
}, [webcamRef]);📝 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.
| const capture = useCallback(() => { | |
| if (webcamRef.current) { | |
| const imageSrc = webcamRef.current.getScreenshot(); | |
| setCapturedImageUrl(imageSrc); | |
| setShowCamera(false); | |
| } | |
| }, [webcamRef]); | |
| const capture = useCallback(() => { | |
| if (webcamRef.current) { | |
| const imageSrc = webcamRef.current.getScreenshot(); | |
| if (imageSrc) { | |
| setCapturedImageUrl(imageSrc); | |
| setShowCamera(false); | |
| } | |
| } | |
| }, [webcamRef]); |
🤖 Prompt for AI Agents
In frontend/src/components/WebCam/WebCamComponent.tsx around lines 68 to 74, the
capture callback calls webcamRef.current.getScreenshot() without checking its
return value which can be null; modify the function to store the result in a
variable, check if the result is non-null before calling setCapturedImageUrl and
setShowCamera(false), and handle the null case (e.g., keep camera open and
optionally set an error/notification state) so we never set capturedImageUrl to
null.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/src/components/WebCam/WebCamComponent.tsx (3)
123-131: Make the webcam responsive and handle permission errors; optionally reduce image size.Fixed 500×500 breaks on small screens; add responsive classes and error handler. Also consider screenshotQuality to reduce payload.
<Webcam audio={false} - height={500} ref={webcamRef} screenshotFormat="image/jpeg" - width={500} + screenshotQuality={0.8} videoConstraints={videoConstraints} - className="rounded-lg border" + className="rounded-lg border w-full max-w-md h-auto" + onUserMediaError={() => + dispatch( + showInfoDialog({ + title: 'Camera Access Denied', + message: + 'We could not access your camera. Please grant permission or use file upload.', + variant: 'error', + }), + ) + } />
69-75: Null-check getScreenshot() to avoid null state and user confusion.getScreenshot() may return null; handle it and keep the camera open with an error hint. Also add dispatch to the hook deps.
- const capture = useCallback(() => { - if (webcamRef.current) { - const imageSrc = webcamRef.current.getScreenshot(); - setCapturedImageUrl(imageSrc); - setShowCamera(false); - } - }, [webcamRef]); + const capture = useCallback(() => { + const cam = webcamRef.current; + if (!cam) return; + const imageSrc = cam.getScreenshot(); + if (imageSrc) { + setCapturedImageUrl(imageSrc); + setShowCamera(false); + } else { + dispatch( + showInfoDialog({ + title: 'Capture Failed', + message: 'Unable to capture image. Please try again.', + variant: 'error', + }), + ); + } + }, [webcamRef, dispatch]);
82-98: Don’t store large base64 in Redux; also send raw base64 (strip data URL prefix) and only close dialog after kicking off search.Avoid putting data URLs into Redux; compute payload locally and pass to mutate. Close only when a valid capture exists.
- const handleSearchCapturedImage = () => { - onClose(); - if (capturedImageUrl) { - dispatch(startSearch(capturedImageUrl)); - dispatch(showLoader('Searching faces...')); - getSearchImagesBase64(capturedImageUrl); - } else { - dispatch( - showInfoDialog({ - title: 'Capture Failed', - message: 'An unexpected error occurred during capture.', - variant: 'error', - }), - ); - handleClose(); - } - }; + const handleSearchCapturedImage = () => { + if (!capturedImageUrl) { + dispatch( + showInfoDialog({ + title: 'Capture Failed', + message: 'An unexpected error occurred during capture.', + variant: 'error', + }), + ); + return; + } + // Strip "data:image/*;base64," if present + const payload = + capturedImageUrl.includes(',') + ? capturedImageUrl.split(',')[1] + : capturedImageUrl; + dispatch(showLoader('Searching faces...')); + getSearchImagesBase64(payload); + onClose(); + };If other UI depends on searchSlice.active, consider a lightweight action (e.g., startSearchFromCamera()) that sets active without storing the base64.
🧹 Nitpick comments (1)
frontend/src/components/WebCam/WebCamComponent.tsx (1)
35-36: Prevent duplicate submissions: disable actions while the mutation is running.Expose isLoading from the mutation and disable Capture/Retake/Search buttons during requests.
- const { mutate: getSearchImagesBase64 } = usePictoMutation({ + const { mutate: getSearchImagesBase64, isLoading } = usePictoMutation({ mutationFn: async (base64_data: string) => fetchSearchedFacesBase64({ base64_data }),- <Button onClick={capture} className="w-40"> + <Button onClick={capture} className="w-40" disabled={isLoading}> Capture Photo </Button>- <Button + <Button onClick={handleRetake} variant="outline" - className="flex items-center gap-2" + className="flex items-center gap-2" + disabled={isLoading} >- <Button + <Button onClick={handleSearchCapturedImage} - className="flex items-center gap-2" + className="flex items-center gap-2" + disabled={isLoading} >Also applies to: 132-135, 144-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Navigation/Navbar/Navbar.tsx(1 hunks)frontend/src/components/WebCam/WebCamComponent.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/Navigation/Navbar/Navbar.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/WebCam/WebCamComponent.tsx (7)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/face_clusters.ts (1)
fetchSearchedFacesBase64(58-66)frontend/src/types/Media.ts (1)
Image(13-22)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/ui/dialog.tsx (5)
Dialog(131-131)DialogContent(133-133)DialogHeader(136-136)DialogTitle(139-139)DialogDescription(134-134)
🔇 Additional comments (1)
frontend/src/components/WebCam/WebCamComponent.tsx (1)
17-18: <!-- [scratchpad_start]
[task verify_barrel_export 2/2]
[observations]
- Barrel index file: frontend/src/api/api-functions/index.ts
- Contains: export * from './face_clusters';
- fetchSearchedFacesBase64 is exported from face_clusters.ts:58 with export const
- The barrel correctly re-exports via wildcard export from face_clusters
[analysis]- fetchSearchedFacesBase64 IS properly re-exported from the barrel '@/api/api-functions'
- The import in WebCamComponent.tsx:17 is correct and will resolve successfully
- The original review comment's concern is unfounded
- This is not an issue; the code is correct as-is
[done]- Verification complete: the barrel export is correct and the import will work as expected
[scratchpad_start] -->No action needed — the barrel export is correctly configured.
The barrel index file (
frontend/src/api/api-functions/index.ts) includesexport * from './face_clusters';, which re-exportsfetchSearchedFacesBase64. The import in WebCamComponent.tsx is correct and will resolve as expected at build time.
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)
frontend/src/components/Dialog/FaceSearchDialog.tsx (1)
80-89: Navigation occurs before file selection completes.Similar to the issue previously fixed in
handleWebCam, the current flow navigates to the home page before the user selects a file. If the user cancels the file picker, they end up on the home page without having accomplished anything.Apply this diff to improve the UX flow:
const handlePickFile = async () => { - navigate(`/${ROUTES.HOME}`); const filePath = await pickSingleFile(); if (filePath) { + navigate(`/${ROUTES.HOME}`); setIsDialogOpen(false); dispatch(startSearch(filePath)); dispatch(showLoader('Searching faces...')); getSearchImages(filePath); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Dialog/FaceSearchDialog.tsx(6 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/Navigation/Navbar/Navbar.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/Dialog/FaceSearchDialog.tsx (2)
frontend/src/constants/routes.ts (1)
ROUTES(1-11)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tauri Build Check (windows-latest)
🔇 Additional comments (1)
frontend/src/components/Dialog/FaceSearchDialog.tsx (1)
62-79: Past review feedback properly addressed.The resource leak and UX flow issues flagged in the previous review have been correctly resolved. The implementation now:
- Stops stream tracks immediately after permission check (preventing resource leak)
- Only navigates and shows camera on successful permission grant
- Keeps user on current page and shows error dialog if permission denied
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src-tauri/Entitlements.plist(1 hunks)frontend/src-tauri/tauri.conf.json(2 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src-tauri/Entitlements.plist
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src-tauri/tauri.conf.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
🔇 Additional comments (1)
frontend/src/components/Navigation/Navbar/Navbar.tsx (1)
37-41: Verify the removal of the fallback image is intentional.The previous implementation used
|| 'photo.png'as a fallback whenconvertFileSrc(queryImage)returned a falsy value. This fallback has been removed. Ensure thatconvertFileSrc(queryImage)always returns a valid string or that displaying an image with an empty/undefinedsrcis acceptable in your use case.If a fallback is still needed, consider adding it back to the non-base64 branch:
src={ queryImage?.startsWith('data:') ? queryImage - : convertFileSrc(queryImage) + : (convertFileSrc(queryImage) || 'photo.png') }
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src-tauri/Entitlements.plist(1 hunks)frontend/src-tauri/Info.plist(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src-tauri/Entitlements.plist
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
backend/app/routes/face_clusters.py
Outdated
| return perform_face_search(image_path) | ||
|
|
||
|
|
||
| @router.post("/face-search-base64") |
There was a problem hiding this comment.
No need to create a separate route for this. Just use FastAPI's query parameters to define what is the input format, whether it is a path or a base64 string. Use the /face-search route only.
Accordingly update the Request and Response Schemas.
| onSuccess: (data) => { | ||
| const result = data?.data as Image[]; | ||
| dispatch(hideLoader()); | ||
| handleClose(); | ||
| if (result && result.length > 0) { | ||
| dispatch(setResults(result)); | ||
| } else { | ||
| dispatch(clearSearch()); | ||
| dispatch( | ||
| showInfoDialog({ | ||
| title: 'No Matches Found', | ||
| message: | ||
| "We couldn't find any matching faces in your gallery for this photo.", | ||
| variant: 'info', | ||
| }), | ||
| ); | ||
| } | ||
| }, | ||
| onError: () => { | ||
| dispatch(hideLoader()); | ||
| handleClose(); | ||
| dispatch( | ||
| showInfoDialog({ | ||
| title: 'Search Failed', | ||
| message: 'There was an error while searching for faces.', | ||
| variant: 'error', | ||
| }), | ||
| ); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Instead of these functions, use this hook: hooks/useMutationFeedback.ts To display messages and show a loader. This keeps the code cleaner. Refer to src/hooks/useFolderOperations.tsx for an example.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/components/Navigation/Navbar/Navbar.tsx (1)
37-41: Previous issue resolved correctly.The change from
includes('base64')tostartsWith('data:')properly addresses the past review concern. This is the correct and robust way to distinguish data URLs from filesystem paths, eliminating false positives from paths containing the substring "base64".
🧹 Nitpick comments (1)
frontend/src/api/apiEndpoints.ts (1)
7-8: Refactor to use a function pattern for better maintainability.Hardcoding query parameters directly in endpoint URLs creates duplication and violates the DRY principle. This is inconsistent with the existing pattern in this file (lines 9-10), which uses functions to handle dynamic parameters.
Apply this diff to refactor using a function pattern:
- searchForFaces: '/face-clusters/face-search?input_type=path', - searchForFacesBase64: '/face-clusters/face-search?input_type=base64', + searchForFaces: (inputType: 'path' | 'base64') => `/face-clusters/face-search?input_type=${inputType}`,Then update call sites to pass the input type:
searchForFaces('path')for filesystem pathssearchForFaces('base64')for base64 dataThis approach:
- Eliminates duplication of the base URL
- Provides type safety for the
input_typeparameter- Maintains consistency with the existing function-based endpoint pattern
- Simplifies maintenance if the base URL changes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/routes/face_clusters.py(3 hunks)backend/app/schemas/images.py(1 hunks)docs/backend/backend_python/openapi.json(5 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/schemas/images.py
- docs/backend/backend_python/openapi.json
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/routes/face_clusters.py (2)
backend/app/schemas/images.py (3)
AddSingleBase64ImageRequest(16-17)AddSingleImageRequest(12-13)InputType(6-8)backend/app/utils/faceSearch.py (1)
perform_face_search(35-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Backend Tests
🔇 Additional comments (3)
backend/app/routes/face_clusters.py (3)
1-31: LGTM: Imports are appropriate.All new imports are necessary for the consolidated face-search functionality.
215-225: LGTM: Path validation is correct.File existence check and error handling are appropriate.
259-259: LGTM: Clean delegation to shared utility.The return statement appropriately delegates face search logic to the centralized
perform_face_searchutility. The Enum type forinput_typeensures only valid values are accepted, so no additional branch handling is needed.
backend/app/routes/face_clusters.py
Outdated
| def face_tagging(payload: AddSingleImageRequest | AddSingleBase64ImageRequest, input_type: Annotated[InputType, Query(description="Choose input type: 'path' or 'base64'")] = InputType.path): | ||
| image_path = None |
There was a problem hiding this comment.
Fix payload validation: union type with query parameter discriminator will fail.
The function accepts a union type AddSingleImageRequest | AddSingleBase64ImageRequest, but the discriminator is the input_type query parameter. This creates a validation mismatch where FastAPI validates the body against both schemas independently of the query parameter, but runtime code assumes the payload type matches input_type.
Problem scenario:
- Request:
?input_type=base64with body{"path": "/some/path"} - FastAPI validation: ✅ Passes (matches
AddSingleImageRequest) - Runtime line 228: ❌
AttributeErrorwhen accessingpayload.base64_dataon anAddSingleImageRequestinstance
Solutions:
- Preferred: Use discriminated union with
input_typefield in payload body:
-def face_tagging(payload: AddSingleImageRequest | AddSingleBase64ImageRequest, input_type: Annotated[InputType, Query(description="Choose input type: 'path' or 'base64'")] = InputType.path):
+from typing import Union
+from pydantic import Field
+
+# Update schemas to include discriminator field
+class AddSingleImageRequest(BaseModel):
+ input_type: Literal["path"] = Field(default="path")
+ path: str
+
+class AddSingleBase64ImageRequest(BaseModel):
+ input_type: Literal["base64"] = Field(default="base64")
+ base64_data: str
+
+def face_tagging(payload: Annotated[Union[AddSingleImageRequest, AddSingleBase64ImageRequest], Field(discriminator="input_type")]):
+ input_type = payload.input_type
image_path = None- Alternative: Use a single unified schema with optional fields:
+class FaceSearchRequest(BaseModel):
+ path: Optional[str] = None
+ base64_data: Optional[str] = None
+
-def face_tagging(payload: AddSingleImageRequest | AddSingleBase64ImageRequest, input_type: Annotated[InputType, Query(description="Choose input type: 'path' or 'base64'")] = InputType.path):
+def face_tagging(payload: FaceSearchRequest, input_type: Annotated[InputType, Query(description="Choose input type: 'path' or 'base64'")] = InputType.path):Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/app/routes/face_clusters.py around lines 212-213, the handler accepts
a union body type but uses the query param input_type as discriminator which
causes FastAPI to validate the body against either schema regardless of
input_type and leads to AttributeError at runtime; fix by making the payload a
discriminated union with an explicit input_type field inside the request body
(or remove the query param and rely on the body discriminator), updating the
Pydantic models to include a literal input_type field and use
typing.Annotated/Field(discriminator="input_type") so FastAPI validates the
correct branch; alternatively, replace the union with a single unified schema
containing optional path and base64_data fields and perform manual validation at
the start of the handler based on the query input_type, returning 400 on
mismatch.
| elif input_type == InputType.base64: | ||
| base64_data = payload.base64_data | ||
| if not base64_data: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail=ErrorResponse( | ||
| success=False, | ||
| error="No base64 data", | ||
| message="Base64 image data is required.", | ||
| ).model_dump(), | ||
| ) | ||
| 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"], | ||
| ) | ||
| ) | ||
|
|
||
| return GetAllImagesResponse( | ||
| success=True, | ||
| message=f"Successfully retrieved {len(matches)} images", | ||
| data=matches, | ||
| try: | ||
| image_bytes = base64.b64decode(base64_data.split(",")[-1]) | ||
| except (Base64Error, ValueError): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail=ErrorResponse( | ||
| success=False, | ||
| error="Invalid base64 data", | ||
| message="The provided base64 image data is malformed or invalid.", | ||
| ).model_dump(), | ||
| ) | ||
| finally: | ||
| fd.close() | ||
| fn.close() | ||
| image_id = str(uuid.uuid4())[:8] | ||
| temp_dir = "temp_uploads" | ||
| os.makedirs(temp_dir, exist_ok=True) | ||
| local_image_path = os.path.join(temp_dir, f"{image_id}.jpeg") | ||
|
|
||
| with open(local_image_path, "wb") as f: | ||
| f.write(image_bytes) | ||
|
|
||
| image_path = local_image_path |
There was a problem hiding this comment.
Critical: Resource leak—temporary files are never cleaned up.
The base64 flow writes a temporary file to disk (line 252-255) but never deletes it. Previous versions had a finally block that handled cleanup, but this refactored version omits it entirely, causing a resource leak where temp files accumulate indefinitely.
Wrap the perform_face_search call with cleanup logic:
image_path = local_image_path
- return perform_face_search(image_path)
+ try:
+ return perform_face_search(image_path)
+ finally:
+ # Cleanup temporary base64 file
+ if input_type == InputType.base64 and image_path and os.path.exists(image_path):
+ os.remove(image_path)Major: Hardcoded .jpeg extension may not match actual image format.
Line 252 uses .jpeg regardless of the actual image format. The base64 data URL prefix typically indicates the format (e.g., data:image/png;base64,...). Saving with the wrong extension can cause issues with libraries that rely on file extensions.
Detect the format from the data URL prefix:
+ # Extract format from data URL (e.g., "data:image/png;base64,...")
+ format_match = base64_data.split(";")[0].split("/")[-1] if ";" in base64_data else "jpeg"
+ extension = format_match if format_match in ["jpeg", "jpg", "png", "gif", "webp"] else "jpeg"
image_id = str(uuid.uuid4())[:8]
temp_dir = "temp_uploads"
os.makedirs(temp_dir, exist_ok=True)
- local_image_path = os.path.join(temp_dir, f"{image_id}.jpeg")
+ local_image_path = os.path.join(temp_dir, f"{image_id}.{extension}")Major: Hardcoded temp directory path.
Line 250 hardcodes "temp_uploads". This should be configurable via environment variables or a config module for flexibility across environments (dev, staging, production).
Move to configuration:
# In config.py or settings
TEMP_UPLOAD_DIR = os.getenv("TEMP_UPLOAD_DIR", "temp_uploads")
# In route
temp_dir = TEMP_UPLOAD_DIROptional: 8-character UUID increases collision risk.
Line 249 uses only 8 characters of a UUID. While collisions are unlikely in typical usage, high-volume scenarios or long-running services could encounter duplicates. Consider using the full UUID or at least 16 characters.
- image_id = str(uuid.uuid4())[:8]
+ image_id = str(uuid.uuid4()) # or [:16] for a balance🤖 Prompt for AI Agents
In backend/app/routes/face_clusters.py around lines 227 to 257, the base64
handling writes a temp file but never cleans it up, always uses a hardcoded
".jpeg" extension, and uses a hardcoded temp dir and short 8-char UUID; fix by
(1) deriving image format from the base64 data URL prefix and choose an
appropriate extension, (2) use a configured temp directory (read from config or
env var like TEMP_UPLOAD_DIR) instead of the literal "temp_uploads", (3) wrap
the perform_face_search call (or the surrounding logic that uses the temp file)
in try/finally so the local_image_path is removed in the finally block to
prevent leaks, and (4) replace the 8-character slice with a less collision-prone
identifier (full UUID or at least 16 chars).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/routes/face_clusters.py (1)
208-214: Add a response_model to document the 200 payload.The handler returns GetAllImagesResponse, but the route lacks response_model, producing an empty 200 schema in OpenAPI. Add the model to keep clients typed.
-@router.post( - "/face-search", - responses={code: {"model": ErrorResponse} for code in [400, 500]}, -) -def face_tagging(payload: FaceSearchRequest, input_type: Annotated[InputType, Query(description="Choose input type: 'path' or 'base64'")] = InputType.path): +@router.post( + "/face-search", + response_model=GetAllImagesResponse, + responses={code: {"model": ErrorResponse} for code in [400, 500]}, +) +def face_tagging( + payload: FaceSearchRequest, + input_type: Annotated[InputType, Query(description="Choose input type: 'path' or 'base64'")] = InputType.path, +):docs/backend/backend_python/openapi.json (1)
1069-1076: Add response_model=GetAllImagesResponse to the face-search route decorator.The
face_tagging()function returnsperform_face_search(image_path), which returnsGetAllImagesResponse. The@router.post()decorator at lines 208–210 lacksresponse_model, causing the OpenAPI 200 response schema to be empty{}. Addresponse_model=GetAllImagesResponseto the decorator and regenerate the OpenAPI spec.
♻️ Duplicate comments (2)
frontend/src/components/WebCam/WebCamComponent.tsx (2)
80-85: Don’t store base64 image in Redux.Large data URLs bloat state/DevTools. Pass base64 directly to the mutation and keep it in component state.
const handleSearchCapturedImage = () => { onClose(); if (capturedImageUrl) { - dispatch(startSearch(capturedImageUrl)); - getSearchImagesBase64.mutate(capturedImageUrl); + // Option A: lightweight start flag + dispatch(startSearch('camera')); // or omit entirely + getSearchImagesBase64.mutate(capturedImageUrl); } else {
67-73: Null-check getScreenshot() result.getScreenshot() may return null; handle it to avoid setting null into state and toggling UI incorrectly.
const capture = useCallback(() => { if (webcamRef.current) { - const imageSrc = webcamRef.current.getScreenshot(); - setCapturedImageUrl(imageSrc); - setShowCamera(false); + const imageSrc = webcamRef.current.getScreenshot(); + if (imageSrc) { + setCapturedImageUrl(imageSrc); + setShowCamera(false); + } else { + dispatch( + showInfoDialog({ + title: 'Capture Failed', + message: 'Unable to capture image. Please try again.', + variant: 'error', + }), + ); + } } }, [webcamRef]);
🧹 Nitpick comments (6)
backend/app/routes/face_clusters.py (2)
261-267: Temp file handling: prefer tempfile + configurable dir + stronger IDs.
- Hardcoded "temp_uploads" and 8-char UUID increase collision/config risk.
- Use tempfile in a configured dir; use full UUID.
- format_match = base64_data.split(";")[0].split("/")[-1] if ";" in base64_data else "jpeg" - extension = format_match if format_match in ["jpeg", "jpg", "png", "gif", "webp"] else "jpeg" - image_id = str(uuid.uuid4())[:8] - temp_dir = "temp_uploads" - os.makedirs(temp_dir, exist_ok=True) - local_image_path = os.path.join(temp_dir, f"{image_id}.{extension}") + format_match = base64_data.split(";")[0].split("/")[-1] if ";" in base64_data else "jpeg" + extension = format_match if format_match in {"jpeg", "jpg", "png", "gif", "webp"} else "jpeg" + image_id = str(uuid.uuid4()) + temp_dir = os.getenv("TEMP_UPLOAD_DIR", "temp_uploads") + os.makedirs(temp_dir, exist_ok=True) + import tempfile + fd, local_image_path = tempfile.mkstemp(prefix=f"{image_id}_", suffix=f".{extension}", dir=temp_dir) + os.close(fd)
261-269: Validate actual image type from bytes, not just data URL.Headers can lie. Probe bytes (e.g., Pillow or imghdr) and normalize extension accordingly.
If Pillow is available:
+ from PIL import Image + from io import BytesIO + try: + fmt = Image.open(BytesIO(image_bytes)).format # e.g., 'JPEG', 'PNG' + extension = fmt.lower() if fmt else extension + except Exception: + pass # keep derived extensiondocs/backend/backend_python/openapi.json (3)
1045-1056: Minor: Parameter description duplication.
descriptionappears both on the parameter and inside its schema. Keep only at parameter level for clarity.
1516-1543: Consider schema constraints for FaceSearchRequest.If input_type=path then path should be required; if base64 then base64_data should be required. FastAPI can’t express cross-parameter requirements here; document with examples or switch to a discriminated body schema.
1039-1046: Rename summary to “Face Search”.Align resource name and UI (“Face Search” vs “Face Tagging”).
frontend/src/components/WebCam/WebCamComponent.tsx (1)
125-131: Optional: reduce screenshot size to improve UX/perf.Set screenshotQuality (e.g., 0.8) to reduce payload size; faster upload and less memory.
<Webcam audio={false} ref={webcamRef} - screenshotFormat="image/jpeg" + screenshotFormat="image/jpeg" + screenshotQuality={0.8} videoConstraints={videoConstraints} className="w-full max-w-md rounded-lg border" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/routes/face_clusters.py(3 hunks)backend/app/schemas/images.py(1 hunks)docs/backend/backend_python/openapi.json(5 hunks)frontend/src/components/WebCam/WebCamComponent.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/schemas/images.py
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/WebCam/WebCamComponent.tsx (6)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/face_clusters.ts (1)
fetchSearchedFacesBase64(58-66)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)frontend/src/types/Media.ts (1)
Image(13-22)frontend/src/features/searchSlice.ts (3)
setResults(24-26)clearSearch(27-31)startSearch(20-23)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)
backend/app/routes/face_clusters.py (2)
backend/app/schemas/images.py (2)
FaceSearchRequest(11-13)InputType(6-8)backend/app/utils/faceSearch.py (1)
perform_face_search(35-106)
🔇 Additional comments (2)
backend/app/routes/face_clusters.py (1)
273-277: Cleanup looks good.Base64 temp file is removed in finally; path inputs are preserved. LGTM.
Optional: add best-effort logging on cleanup failures.
frontend/src/components/WebCam/WebCamComponent.tsx (1)
33-38: Import path is correctly configured.The barrel index file at
frontend/src/api/api-functions/index.tsproperly re-exportsfetchSearchedFacesBase64fromface_clusters.tsviaexport * from './face_clusters', making the import from@/api/api-functionsvalid and functional. No changes needed.
| const getSearchImagesBase64 = usePictoMutation({ | ||
| mutationFn: async (base64_data: string) => | ||
| fetchSearchedFacesBase64({ base64_data }), | ||
| }); |
There was a problem hiding this comment.
Endpoint mismatch: use unified /face-search with input_type=base64.
Backend now exposes POST /face-clusters/face-search with query input_type. Calling fetchSearchedFacesBase64 (likely hitting a removed base64-specific route) will 404.
- const getSearchImagesBase64 = usePictoMutation({
- mutationFn: async (base64_data: string) =>
- fetchSearchedFacesBase64({ base64_data }),
- });
+ const getSearchImagesBase64 = usePictoMutation({
+ mutationFn: async (base64_data: string) =>
+ // Prefer a dedicated api function: fetchFaceSearch({ base64_data }, { input_type: 'base64' })
+ apiClient.post('/face-clusters/face-search?input_type=base64', {
+ base64_data,
+ }).then(r => r.data),
+ });Follow-up: update api-functions/face_clusters.ts to export a generic fetchFaceSearch(body, params) and change callers. I can draft that if helpful.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/app/routes/face_clusters.py (2)
243-253: Add size limit validation for base64 payloads.The code doesn't validate the size of
base64_databefore decoding, which could allow memory exhaustion attacks with oversized payloads.Apply this diff to add size validation:
+ # Define max size (e.g., 10MB base64 ≈ 14MB encoded) + MAX_BASE64_SIZE = int(os.getenv("MAX_BASE64_IMAGE_SIZE", 14_000_000)) + elif input_type == InputType.base64: base64_data = payload.base64_data if not base64_data: raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=ErrorResponse( success=False, error="No base64 data", message="Base64 image data is required.", ).model_dump(), ) + + # Validate size before decoding + if len(base64_data) > MAX_BASE64_SIZE: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, + error="Payload too large", + message="Base64 image exceeds maximum allowed size.", + ).model_dump(), + )Based on learnings
220-241: Critical: Validate file paths against an allowlist to prevent path traversal.The code accepts arbitrary filesystem paths without validation, allowing potential access to sensitive files outside the intended media directory. An attacker could provide paths like
/etc/passwdor use../to escape the intended directory.Apply this diff to add path validation:
+from pathlib import Path + +# At the top of the function or in config +MEDIA_ROOT = os.getenv("MEDIA_ROOT", os.path.abspath("uploads")) + if input_type == InputType.path: local_file_path = payload.path if not local_file_path: raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=ErrorResponse( success=False, error="No Image path provided ", message="image path is required.", ).model_dump(), ) + + # Resolve path and validate against allowlist + try: + resolved_path = Path(local_file_path).resolve() + media_root = Path(MEDIA_ROOT).resolve() + + # Check if path is within allowed directory + if not resolved_path.is_relative_to(media_root): + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, + error="Path not allowed", + message="The provided path is outside the allowed media directory.", + ).model_dump(), + ) + except ValueError: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, + error="Invalid path", + message="The provided path is invalid.", + ).model_dump(), + ) + - if not os.path.isfile(local_file_path): + if not resolved_path.is_file(): 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(), ) - image_path = payload.path + + # Check file is readable + if not os.access(resolved_path, os.R_OK): + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, + error="Unreadable file", + message="The server cannot read the provided file.", + ).model_dump(), + ) + + image_path = str(resolved_path)Based on learnings
🧹 Nitpick comments (3)
frontend/src/api/apiEndpoints.ts (1)
5-11: Consider adding JSDoc documentation for the face cluster endpoints.Adding documentation would improve maintainability by clarifying the expected usage, input types, and any request format requirements for each endpoint.
Apply this diff to add documentation:
+/** + * Face clustering and search endpoints. + */ export const faceClustersEndpoints = { + /** Retrieves all face clusters */ getAllClusters: '/face-clusters/', + /** Searches for faces using file path input */ searchForFaces: '/face-clusters/face-search?input_type=path', + /** Searches for faces using base64-encoded image data */ searchForFacesBase64: '/face-clusters/face-search?input_type=base64', + /** Updates the name of a specific cluster */ renameCluster: (clusterId: string) => `/face-clusters/${clusterId}`, + /** Retrieves all images belonging to a specific cluster */ getClusterImages: (clusterId: string) => `/face-clusters/${clusterId}/images`, };backend/app/routes/face_clusters.py (2)
274-277: Use configurable temp directory path.The temp directory is hardcoded to
"temp_uploads", which reduces flexibility across different environments (dev, staging, production).Apply this diff to use a configurable path:
- image_id = str(uuid.uuid4())[:8] - temp_dir = "temp_uploads" + image_id = str(uuid.uuid4())[:8] + temp_dir = os.getenv("TEMP_UPLOAD_DIR", "temp_uploads") os.makedirs(temp_dir, exist_ok=True) local_image_path = os.path.join(temp_dir, f"{image_id}.{extension}")Based on learnings
274-274: Consider using full UUID for temp file names.The 8-character UUID prefix increases collision risk in high-volume scenarios. While unlikely for temporary files with immediate cleanup, using the full UUID provides stronger guarantees.
- image_id = str(uuid.uuid4())[:8] + image_id = str(uuid.uuid4())Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
backend/app/routes/face_clusters.py(3 hunks)frontend/package.json(1 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/package.json
- frontend/src/components/Navigation/Navbar/Navbar.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/routes/face_clusters.py (2)
backend/app/schemas/images.py (3)
FaceSearchRequest(11-13)InputType(6-8)ErrorResponse(59-62)backend/app/utils/faceSearch.py (1)
perform_face_search(35-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
🔇 Additional comments (4)
frontend/src/api/apiEndpoints.ts (1)
7-8: LGTM! Backend correctly implements dual input-type handling.Verified that the backend
/face-searchendpoint properly handles bothinput_typequery parameters:
- path: Validates the file exists locally and passes to face search
- base64: Decodes base64 data, saves to temp file, processes it, and cleans up after
The frontend endpoint constants align with the backend implementation, and the approach maintains backward compatibility with the default path-based input type.
backend/app/routes/face_clusters.py (3)
2-7: LGTM! Imports are appropriate.The new imports support the dual-input functionality (path/base64) and proper error handling for base64 decoding.
Also applies to: 26-27
212-218: LGTM! Unified schema approach with query discriminator.The function uses a unified
FaceSearchRequestschema with optional fields and relies on theinput_typequery parameter to determine which field to use. This is a valid approach that addresses previous concerns about payload validation.
284-288: LGTM! Cleanup logic properly prevents resource leaks.The
finallyblock correctly removes temporary files created during base64 processing, with appropriate guards to prevent errors.
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail=ErrorResponse( | ||
| success=False, | ||
| error="No Image path provided ", |
There was a problem hiding this comment.
Fix typo in error message.
There's an extra space at the end of the error message.
Apply this diff:
- error="No Image path provided ",
+ error="No Image path provided",🤖 Prompt for AI Agents
In backend/app/routes/face_clusters.py around line 228, the error message string
"No Image path provided " has an extra trailing space; remove the trailing space
so the message reads "No Image path provided" (update the string literal in that
location).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/routes/face_clusters.py (1)
228-228: Fix: Remove trailing space in error message.- error="No Image path provided ", + error="No Image path provided",
🧹 Nitpick comments (1)
backend/app/routes/face_clusters.py (1)
243-293: Strong base64 handling with room for configuration improvements.The implementation correctly:
- Validates payload presence before processing
- Enforces size limits to prevent memory exhaustion attacks
- Handles decoding errors gracefully with appropriate HTTP status codes
- Detects image format from the data URL prefix
However, two minor configuration issues remain from previous reviews:
Line 286: The
temp_uploadsdirectory is still hardcoded. Consider moving to environment configuration:temp_dir = os.getenv("TEMP_UPLOAD_DIR", "temp_uploads")Line 285: The 8-character UUID prefix has low (but non-zero) collision risk in high-volume scenarios. Using the full UUID or at least 16 characters would be safer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/routes/face_clusters.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/routes/face_clusters.py (2)
backend/app/schemas/images.py (3)
FaceSearchRequest(11-13)InputType(6-8)ErrorResponse(59-62)backend/app/utils/faceSearch.py (1)
perform_face_search(35-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Backend Tests
🔇 Additional comments (3)
backend/app/routes/face_clusters.py (3)
1-31: LGTM: Imports are well-organized and necessary.All new imports (Base64Error, base64, Annotated, Query, FaceSearchRequest, InputType, perform_face_search) are appropriately used in the face search implementation below.
212-217: Good solution: Single route with query parameter discriminator.The implementation correctly addresses the maintainer's feedback to use a single
/face-searchroute with a query parameter to distinguish between path and base64 inputs, rather than separate endpoints.
295-299: Excellent cleanup implementation.The
try/finallypattern correctly ensures temporary files are removed even when exceptions occur. The conditional checkif input_type == InputType.base64 and image_path and os.path.exists(image_path)properly guards against cleanup errors and only removes files created by the base64 path.
| if input_type == InputType.path: | ||
| local_file_path = payload.path | ||
|
|
||
| if not local_file_path: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail=ErrorResponse( | ||
| success=False, | ||
| error="No Image path provided ", | ||
| message="image path is required.", | ||
| ).model_dump(), | ||
| ) | ||
| if not os.path.isfile(local_file_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(), | ||
| ) | ||
| image_path = payload.path |
There was a problem hiding this comment.
🧩 Analysis chain
Validate that only the expected field is provided.
When input_type=path, the code doesn't verify that payload.base64_data is None. A client could send both fields, leading to ambiguous behavior where base64_data is silently ignored. This makes debugging harder and violates the principle of early validation.
Add mutual exclusivity validation at the start of the function:
def face_tagging(
payload: FaceSearchRequest,
input_type: Annotated[
InputType, Query(description="Choose input type: 'path' or 'base64'")
] = InputType.path,
):
image_path = None
+
+ # Validate mutual exclusivity
+ if input_type == InputType.path and payload.base64_data is not None:
+ raise HTTPException(
+ status_code=status.HTTP_400_BAD_REQUEST,
+ detail=ErrorResponse(
+ success=False,
+ error="Invalid request",
+ message="Cannot provide base64_data when input_type is 'path'.",
+ ).model_dump(),
+ )
+
+ if input_type == InputType.base64 and payload.path is not None:
+ raise HTTPException(
+ status_code=status.HTTP_400_BAD_REQUEST,
+ detail=ErrorResponse(
+ success=False,
+ error="Invalid request",
+ message="Cannot provide path when input_type is 'base64'.",
+ ).model_dump(),
+ )
if input_type == InputType.path:Previous security concerns remain unaddressed.
As noted in earlier reviews, the path input still lacks validation against an allowlist/configured media root, which could allow reading arbitrary filesystem paths. Additionally, line 228 has a trailing space in the error message.
🏁 Script executed:
#!/bin/bash
# Examine the face_tagging function and surrounding context
cat backend/app/routes/face_clusters.py | sed -n '200,260p'Length of output: 2047
🏁 Script executed:
#!/bin/bash
# Find the FaceSearchRequest model definition
rg -n "class FaceSearchRequest" --type=py -A 10Length of output: 586
🏁 Script executed:
#!/bin/bash
# Check for any path validation patterns or allowlist usage in the codebase
rg -n "allowlist|MEDIA_ROOT|secure_path|safe.*path" --type=py -C 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for the InputType enum definition
rg -n "class InputType|InputType\s*=" --type=py -A 5Length of output: 299
🏁 Script executed:
#!/bin/bash
# Check for any global path validation or filesystem configuration
rg -n "os\.path\.|file.*path|image.*path" --type=py -B 2 -A 2 | head -50Length of output: 3003
🏁 Script executed:
#!/bin/bash
# Look for configuration files that might define allowed paths
fd -type f -name "*.env*" -o -name "*config*" -o -name "settings*" | grep -E "\.(py|env|yaml|toml|json)$"Length of output: 170
🏁 Script executed:
#!/bin/bash
# Check if there's any validation in middleware or app initialization
rg -n "mount|middleware|startup|app\." --type=py backend/app/ | grep -E "(middleware|mount|startup)" | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify the actual behavior: does base64_data get processed if both are sent?
cat backend/app/routes/face_clusters.py | sed -n '210,300p' | grep -A 5 "elif input_type"Length of output: 238
Validate that only the expected field is provided.
When input_type=path, the code doesn't verify that payload.base64_data is None. A client could send both fields, leading to ambiguous behavior where base64_data is silently ignored. This makes debugging harder and violates the principle of early validation.
Add mutual exclusivity validation at the start of the function:
def face_tagging(
payload: FaceSearchRequest,
input_type: Annotated[
InputType, Query(description="Choose input type: 'path' or 'base64'")
] = InputType.path,
):
image_path = None
+
+ # Validate mutual exclusivity
+ if input_type == InputType.path and payload.base64_data is not None:
+ raise HTTPException(
+ status_code=status.HTTP_400_BAD_REQUEST,
+ detail=ErrorResponse(
+ success=False,
+ error="Invalid request",
+ message="Cannot provide base64_data when input_type is 'path'.",
+ ).model_dump(),
+ )
+
+ if input_type == InputType.base64 and payload.path is not None:
+ raise HTTPException(
+ status_code=status.HTTP_400_BAD_REQUEST,
+ detail=ErrorResponse(
+ success=False,
+ error="Invalid request",
+ message="Cannot provide path when input_type is 'base64'.",
+ ).model_dump(),
+ )
if input_type == InputType.path:Fix trailing space in error message and address remaining path security concerns.
Line 228 contains a trailing space: error="No Image path provided ". Additionally, the path input still lacks validation against an allowlist or configured media root, which could allow reading arbitrary filesystem paths.
Committable suggestion skipped: line range outside the PR's diff.
2 participants
Issue - #502
Video Ref :- https://drive.google.com/file/d/1hjGRv23Jhul-N_f9bcG4MfUmCyuWt3kT/view?usp=sharing
Summary by CodeRabbit
New Features
User Experience
Security & Configuration