Gsoc'25 Frontend Revamp #489
Conversation
…C'25/Frontend-Revamp
…into GSoC'25/Frontend-Revamp
…C'25/Frontend-Revamp
WalkthroughAdds new microservice URL/config and utilities, extends cluster/image/folder DB schemas and APIs (including image listings and base64 previews), starts/mounts an images router and sync microservice, refactors frontend API/Redux/viewer/sidebar/onboarding, and removes numerous legacy UI modules and hooks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as FastAPI App
participant DB as SQLite
participant Sync as Sync Microservice
rect rgba(200,230,255,0.25)
note over App: App startup
App->>DB: ensure tables (including new columns)
App->>Sync: microservice_util_start_sync_service()
alt started
Sync-->>App: started (port 8001)
else failed
Sync-->>App: error logged
end
end
sequenceDiagram
autonumber
actor Client
participant API as FaceClustersRoute
participant DB as SQLite
rect rgba(240,255,220,0.25)
Client->>API: GET /face-clusters/{cluster_id}/images
API->>DB: db_get_cluster_by_id(cluster_id)
DB-->>API: cluster or none
alt cluster exists
API->>DB: db_get_images_by_cluster_id(cluster_id)
DB-->>API: image rows (bbox JSON parsed, confidence, metadata)
API-->>Client: 200 { data: { cluster_id, cluster_name, images[], total_images } }
else missing
API-->>Client: 404 ErrorResponse { error/message }
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 84
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (26)
backend/app/utils/images.py (3)
258-278: Avoid infinite loop at root and fix return type to Optional[int]
- Import
Optionaland change the signature to returnOptional[int]- Terminate the loop when
os.path.dirname(current) == currentto prevent hanging at filesystem rootApply:
-from typing import List, Tuple, Dict +from typing import List, Tuple, Dict, Optional -def image_util_find_folder_id_for_image(image_path: str, folder_path_to_id: Dict[str, int]) -> int: +def image_util_find_folder_id_for_image(image_path: str, folder_path_to_id: Dict[str, int]) -> Optional[int]: """ Find the most specific folder ID for a given image path. @@ - current_path = parent_folder - while current_path: - if current_path in folder_path_to_id: - return folder_path_to_id[current_path] - current_path = os.path.dirname(current_path) + current_path = os.path.abspath(parent_folder) + while True: + if current_path in folder_path_to_id: + return folder_path_to_id[current_path] + parent = os.path.dirname(current_path) + if parent == current_path: + break + current_path = parent return None
141-156: Use relative paths for thumbnails in DB/response
StorethumbnailPathas a relative path (or public URL) and use the absolute path only for I/O. Update the serializer to expose a public URL (e.g./thumbnails/thumbnail_<id>.jpg).
193-208: Improve thumbnail robustness and size handling
- Use high-quality LANCZOS resampling
- Convert all non-RGB modes (
img.mode != "RGB") to RGB- Save JPEG thumbnails with
quality=85, optimize=True, progressive=True- (Optional) Honor EXIF orientation: add
from PIL import ImageOpsandimg = ImageOps.exif_transpose(img)after openingApply:
- with Image.open(image_path) as img: - img.thumbnail(size) + with Image.open(image_path) as img: + img.thumbnail(size, Image.Resampling.LANCZOS) + img = ImageOps.exif_transpose(img) # handle EXIF orientation - # Convert to RGB if the image has an alpha channel or is not RGB - if img.mode in ("RGBA", "P"): - img = img.convert("RGB") + if img.mode != "RGB": + img = img.convert("RGB") - img.save(thumbnail_path, "JPEG") # Always save thumbnails as JPEG + img.save(thumbnail_path, "JPEG", quality=85, optimize=True, progressive=True)backend/app/utils/microservice.py (1)
143-173: Refactor_start_fastapi_serviceto useuvicornwith explicit app and non‐blocking outputs• Replace
python -m fastapi devwithpython -m uvicorn <app_ref>(useSYNC_SVC_APP,SYNC_SVC_HOST,SYNC_SVC_PORT)
• Removeos.chdir; passcwd=service_pathtosubprocess.Popen
• Drop the strayprint(python_executable)and redirectstdout/stderrtoDEVNULLwithstart_new_session=True
• Update the log to reference the configuredhost/portbackend/app/models/FaceDetector.py (1)
22-27: Changedetect_faces’simage_idparameter tostr
Callers pass UUID strings and the database layer usesImageId(a string alias).- def detect_faces(self, image_id: int, image_path: str): + def detect_faces(self, image_id: str, image_path: str):frontend/src/hooks/useQueryExtension.ts (1)
26-37: Don’t accept a QueryClient param; use the provider client.In React Query v5, prefer the context client (useQueryClient) instead of passing a client to useMutation. Drop the queryClient argument and the second parameter to useMutation to avoid API drift.
-import { - useMutation, - DefaultError, - UseMutationOptions, - QueryClient, - UseMutationResult, - useQueryClient, -} from '@tanstack/react-query'; +import { + useMutation, + DefaultError, + UseMutationOptions, + UseMutationResult, + useQueryClient, +} from '@tanstack/react-query'; @@ export function usePictoMutation< @@ >( - options: UseMutationOptions<TData, TError, TVariables, TContext> & { - autoInvalidateTags?: string[]; - }, - queryClient?: QueryClient, + options: UseMutationOptions<TData, TError, TVariables, TContext> & { + autoInvalidateTags?: QueryKey; // treat as a single query key/prefix + }, ): UseMutationResult<TData, TError, TVariables, TContext> & { @@ - const res = useMutation<TData, TError, TVariables, TContext>( - { + const res = useMutation<TData, TError, TVariables, TContext>({ ...defaultOptions, ...options, @@ - }, - queryClient, - ); + });frontend/src/hooks/useImages.ts (1)
24-29: Guard against missing thumbnailPath in localStorage.Avoid returning "null/filename" paths.
export const extractThumbnailPath = (imagePath: string) => { - const thumbnailPath = localStorage.getItem('thumbnailPath'); + const thumbnailPath = localStorage.getItem('thumbnailPath') || ''; const cleanedImagePath = imagePath.replace(/\\/g, '/'); // Replaces all '\' with '/' const thumbnailImageName = cleanedImagePath.split('/').pop() || ''; - return `${thumbnailPath}/${thumbnailImageName}`; + return thumbnailPath ? `${thumbnailPath}/${thumbnailImageName}` : cleanedImagePath; };backend/app/database/face_clusters.py (1)
21-35: Add migration for new face_image_base64 column and helpful indexesCREATE TABLE won’t add new columns on existing DBs. Add ALTER TABLE guard and indexes used by new queries.
def db_create_clusters_table() -> None: @@ - cursor.execute( + cursor.execute( """ CREATE TABLE IF NOT EXISTS face_clusters ( cluster_id TEXT PRIMARY KEY, - cluster_name TEXT, - face_image_base64 TEXT + cluster_name TEXT, + face_image_base64 TEXT ) """ ) + # Migrate existing DBs: add column if missing + cursor.execute("PRAGMA table_info(face_clusters)") + cols = {row[1] for row in cursor.fetchall()} + if "face_image_base64" not in cols: + cursor.execute("ALTER TABLE face_clusters ADD COLUMN face_image_base64 TEXT") + + # Helpful indexes for downstream joins/filters + cursor.execute("CREATE INDEX IF NOT EXISTS idx_faces_cluster_id ON faces(cluster_id)") + cursor.execute("CREATE INDEX IF NOT EXISTS idx_images_path ON images(path)") conn.commit() conn.close()backend/app/utils/face_clusters.py (2)
1-11: Use structured logging instead of printsIntroduce a module logger.
import numpy as np import uuid import json import base64 import cv2 from datetime import datetime @@ from numpy.typing import NDArray +import logging + +logger = logging.getLogger(__name__)
290-302: Guard against zero-norm embeddings to avoid NaNsZero vectors will yield NaNs and break min/argmin later.
- face_norm = face_embedding / np.linalg.norm(face_embedding) + denom = np.linalg.norm(face_embedding) + if denom == 0: + return np.full((cluster_means.shape[0],), 1.0, dtype=float) # max distance + face_norm = face_embedding / denom @@ - cluster_norms = cluster_means / np.linalg.norm(cluster_means, axis=1, keepdims=True) + cluster_denoms = np.linalg.norm(cluster_means, axis=1, keepdims=True) + cluster_denoms[cluster_denoms == 0] = np.inf # zero vector => zero similarity + cluster_norms = cluster_means / cluster_denomsbackend/app/routes/face_clusters.py (4)
1-1: Fix error response shape (don’t nest your envelope under “detail”).Raising HTTPException with a dict puts it under {"detail": {...}}, which breaks your documented ErrorResponse contract. Return JSONResponse with your envelope instead.
Apply:
-from fastapi import APIRouter, HTTPException, status +from fastapi import APIRouter, HTTPException, status +from fastapi.responses import JSONResponse @@ - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, - error="Cluster Not Found", - message=f"Cluster with ID '{cluster_id}' does not exist.", - ).model_dump(), - ) + return JSONResponse( + status_code=status.HTTP_404_NOT_FOUND, + content=ErrorResponse( + success=False, + error="Cluster Not Found", + message=f"Cluster with ID '{cluster_id}' does not exist.", + ).model_dump(), + ) @@ - if not updated: - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=ErrorResponse( - success=False, - error="Update Failed", - message=f"Failed to update cluster '{cluster_id}'.", - ).model_dump(), - ) + if not updated: + return JSONResponse( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + content=ErrorResponse( + success=False, + error="Update Failed", + message=f"Failed to update cluster '{cluster_id}'.", + ).model_dump(), + ) @@ - except ValueError as e: - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail=ErrorResponse( - success=False, - error="Validation Error", - message=str(e), - ).model_dump(), - ) + except ValueError as e: + return JSONResponse( + status_code=status.HTTP_400_BAD_REQUEST, + content=ErrorResponse( + success=False, + error="Validation Error", + message=str(e), + ).model_dump(), + ) @@ - except Exception as e: - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=ErrorResponse( - success=False, - error="Internal server error", - message=f"Unable to retrieve clusters: {str(e)}", - ).model_dump(), - ) + except Exception: + return JSONResponse( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + content=ErrorResponse( + success=False, + error="Internal Server Error", + message="Unable to retrieve clusters.", + ).model_dump(), + ) @@ - if not cluster: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, - error="Cluster Not Found", - message=f"Cluster with ID '{cluster_id}' does not exist.", - ).model_dump(), - ) + if not cluster: + return JSONResponse( + status_code=status.HTTP_404_NOT_FOUND, + content=ErrorResponse( + success=False, + error="Cluster Not Found", + message=f"Cluster with ID '{cluster_id}' does not exist.", + ).model_dump(), + ) @@ - except Exception as e: - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=ErrorResponse( - success=False, - error="Internal server error", - message=f"Unable to retrieve images for cluster: {str(e)}", - ).model_dump(), - ) + except Exception: + return JSONResponse( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + content=ErrorResponse( + success=False, + error="Internal Server Error", + message="Unable to retrieve images for cluster.", + ).model_dump(), + )Also applies to: 43-51, 58-66, 77-85, 126-134, 148-155, 169-176
110-116: Guard against missing face_image_base64 in DB row.Use .get to avoid KeyError when the column/alias is absent.
- face_image_base64=cluster["face_image_base64"], + face_image_base64=cluster.get("face_image_base64"),
90-97: Avoid leaking internal errors to clients.Returning str(e) in the public message can expose internals. Log the exception and send a generic message (as in the diff above).
Want me to add a shared exception handler + logging?
Also applies to: 169-176
1-200: Use Pydantic models for HTTPException.detail instead of raw dicts. Refactor all routes that currently call.model_dump()onErrorResponse(e.g., inimages.py,folders.py,user_preferences.py,facetagging.py,albums.py) to pass the model instance directly (detail=ErrorResponse(…)) and rely on FastAPI’s serialization.frontend/src/components/FolderPicker/DeleteSelectedImagePage.tsx (4)
50-51: RevisitautoInvalidateTags: it doesn’t target['all-images'].Either add a tag that maps to the same query or drop this to avoid double/partial invalidations. Keep the explicit
invalidateQueriesfor['all-images'].- }, - autoInvalidateTags: ['ai-tagging-images', 'ai'], + }, + // Rely on explicit invalidate for ['all-images']; remove unrelated tags to avoid churn.
65-73: Close the dialog on confirm instead of reopening it.
setOpenDialog(true)in the confirm handler reopens the dialog the user just confirmed.- const handleAddSelectedImages = async (isFromDevice: boolean) => { - setOpenDialog(true); + const handleAddSelectedImages = async (isFromDevice: boolean) => { + setOpenDialog(false);
87-87: Handle emptyuniqueTags; avoid uncontrolled value warnings.Default to
''when no tags exist and ensure the radio group value is a string.- const [filterTag, setFilterTag] = useState<string>(uniqueTags[0]); + const [filterTag, setFilterTag] = useState<string>(uniqueTags[0] ?? ''); @@ - <DropdownMenuRadioGroup + <DropdownMenuRadioGroup className="overflow-auto rounded-lg bg-gray-950 text-white" - value={filterTag} + value={filterTag ?? ''} onValueChange={(value) => handleFilterTag(value)} >Also applies to: 146-148
110-112: MakegetImageNameOS-agnostic.Incoming paths can be POSIX or Windows. Split on both.
- const getImageName = (path: string) => { - return path.split('\\').pop() || path; - }; + const getImageName = (path: string) => { + const sepIndex = Math.max(path.lastIndexOf('/'), path.lastIndexOf('\\')); + return sepIndex >= 0 ? path.slice(sepIndex + 1) : path; + };frontend/src/main.tsx (1)
47-51: Avoid unhandled promise rejections when starting server and registering close listenerExplicitly handle the returned promises.
Apply this diff:
-if (isProd() && isTauriEnvironment()) { - onCloseListener(); - console.log('Starting PictoPy Server'); - startServer(); -} +if (isProd() && isTauriEnvironment()) { + void onCloseListener().catch((e) => + console.error('Failed to set close listener:', e), + ); + console.log('Starting PictoPy Server'); + void startServer().catch((e) => + console.error('Failed to start PictoPy Server:', e), + ); +}backend/main.py (1)
95-101: Fix invalid CORS setup: wildcard origins with credentialsUsing allow_origins=["*"] with allow_credentials=True is disallowed and can break CORS. Restrict origins (config-driven) instead.
app.add_middleware( CORSMiddleware, - allow_origins=["*"], # Allows all origins + allow_origins=os.getenv( + "CORS_ALLOW_ORIGINS", + "http://localhost:3000,http://127.0.0.1:3000" + ).split(","), allow_credentials=True, allow_methods=["*"], allow_headers=["*"], )backend/app/database/folders.py (2)
136-142: Close DB connection in db_get_all_folder_ids (current leak).Function returns without closing
conn.-def db_get_all_folder_ids() -> List[FolderId]: - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() - cursor.execute("SELECT folder_id from folders") - rows = cursor.fetchall() - return [row[0] for row in rows] if rows else [] +def db_get_all_folder_ids() -> List[FolderId]: + with sqlite3.connect(DATABASE_PATH) as conn: + cursor = conn.cursor() + cursor.execute("SELECT folder_id from folders") + rows = cursor.fetchall() + return [row[0] for row in rows] if rows else []
244-257: Use absolute paths when resolving parent folder ID.DB stores absolute paths; querying with non-absolute
parent_pathcan miss matches.- parent_path = os.path.dirname(folder_path) + parent_path = os.path.dirname(os.path.abspath(folder_path)) @@ - try: - cursor.execute("SELECT folder_id FROM folders WHERE folder_path = ?", (parent_path,)) + try: + cursor.execute("SELECT folder_id FROM folders WHERE folder_path = ?", (parent_path,))backend/app/routes/folders.py (2)
94-99: Type hint mismatch: folder_id should be str (UUID), not int.Everywhere else folder_id is a string UUID (db, schemas, utils). Align signatures to avoid confusion and future bugs.
-def post_sync_folder_sequence(folder_path: str, folder_id: int, added_folders: List[Tuple[str, str]]): +def post_sync_folder_sequence(folder_path: str, folder_id: str, added_folders: List[Tuple[str, str]]):Also update Line 51 similarly:
-def post_folder_add_sequence(folder_path: str, folder_id: int): +def post_folder_add_sequence(folder_path: str, folder_id: str):
145-152: Use 403 Forbidden for permission issues instead of 401 Unauthorized.401 implies missing/invalid auth; permission-denied fits 403. Aligning status codes improves API semantics and client handling.
- raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, detail=ErrorResponse( success=False, error="Permission denied", message="The app does not have read permission for the specified folder", ).model_dump(), )Note: Consider updating the same semantic in app.utils.folders.folder_util_get_filesystem_direct_child_folders for consistency.
docs/backend/backend_python/openapi.json (2)
1-2550: Add a global security scheme and per-op security to satisfy Checkov CKV_OPENAPI_4/5.If auth is planned, define a security scheme and reference it; if not, explicitly set an empty security array to indicate no auth.
Minimal example (top-level snippets):
"openapi": "3.1.0", "info": { ... }, + "components": { + "securitySchemes": { + "ApiKeyAuth": { "type": "apiKey", "in": "header", "name": "Authorization" } + }, + "schemas": { ... existing ... } + }, - "components": { "schemas": { ... } } + "security": [ { "ApiKeyAuth": [] } ],Or add
"security": []explicitly if unauthenticated.
1261-1301: Avoid embedding base64 thumbnails inline in list responses.ClusterMetadata requires face_image_base64, which can bloat payloads. Make it optional or move to a dedicated thumbnail endpoint; include a URL or hash in listing.
- "required": ["cluster_id", "cluster_name", "face_image_base64", "face_count"] + "required": ["cluster_id", "cluster_name", "face_count"]
| # Microservice URLs | ||
| SYNC_MICROSERVICE_URL = "http://localhost:8001/api/v1" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make SYNC_MICROSERVICE_URL configurable via environment.
Avoid hardcoding localhost; allow deployments to override and normalize trailing slash.
-# Microservice URLs
-SYNC_MICROSERVICE_URL = "http://localhost:8001/api/v1"
+# Microservice URLs
+import os
+SYNC_MICROSERVICE_URL = os.getenv("SYNC_MICROSERVICE_URL", "http://localhost:8001/api/v1").rstrip("/")📝 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.
| # Microservice URLs | |
| SYNC_MICROSERVICE_URL = "http://localhost:8001/api/v1" | |
| # Microservice URLs | |
| import os | |
| SYNC_MICROSERVICE_URL = os.getenv("SYNC_MICROSERVICE_URL", "http://localhost:8001/api/v1").rstrip("/") |
🤖 Prompt for AI Agents
In backend/app/config/settings.py around lines 4 to 6, SYNC_MICROSERVICE_URL is
hardcoded; change it to read from an environment variable (e.g.,
os.getenv("SYNC_MICROSERVICE_URL")) with the current value as a safe default,
and normalize the value to remove any trailing slash so the URL is consistent;
ensure os is imported and add a small helper or inline logic to strip a trailing
slash before assigning the constant.
| def db_get_images_by_cluster_id(cluster_id: ClusterId) -> List[Dict[str, Union[str, int]]]: | ||
| """ | ||
| Get all images that contain faces belonging to a specific cluster. | ||
|
|
||
| Args: | ||
| cluster_id: The ID of the cluster to get images for | ||
|
|
||
| Returns: | ||
| List of dictionaries containing image data with face information | ||
| """ | ||
| conn = sqlite3.connect(DATABASE_PATH) |
There was a problem hiding this comment.
Return type omits bbox dict and nullable fields
Signature should include Optional[dict] for bbox and Optional[str] for metadata/thumbnail.
-from typing import Optional, List, Dict, TypedDict, Union
+from typing import Optional, List, Dict, TypedDict, Union, Any
@@
-def db_get_images_by_cluster_id(cluster_id: ClusterId) -> List[Dict[str, Union[str, int]]]:
+def db_get_images_by_cluster_id(cluster_id: ClusterId) -> List[Dict[str, Union[str, int, None, Dict[str, Any]]]]:📝 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.
| def db_get_images_by_cluster_id(cluster_id: ClusterId) -> List[Dict[str, Union[str, int]]]: | |
| """ | |
| Get all images that contain faces belonging to a specific cluster. | |
| Args: | |
| cluster_id: The ID of the cluster to get images for | |
| Returns: | |
| List of dictionaries containing image data with face information | |
| """ | |
| conn = sqlite3.connect(DATABASE_PATH) | |
| from typing import Optional, List, Dict, TypedDict, Union, Any | |
| def db_get_images_by_cluster_id(cluster_id: ClusterId) -> List[Dict[str, Union[str, int, None, Dict[str, Any]]]]: | |
| """ | |
| Get all images that contain faces belonging to a specific cluster. | |
| Args: | |
| cluster_id: The ID of the cluster to get images for | |
| Returns: | |
| List of dictionaries containing image data with face information | |
| """ | |
| conn = sqlite3.connect(DATABASE_PATH) |
backend/app/database/images.py
Outdated
| for image_id, path, folder_id, thumbnail_path, metadata, is_tagged, tag_name in results: | ||
| if image_id not in images_dict: | ||
| images_dict[image_id] = {"id": image_id, "path": path, "folder_id": folder_id, "thumbnailPath": thumbnail_path, "metadata": metadata, "isTagged": bool(is_tagged), "tags": []} | ||
|
|
||
| # Add tag if it exists | ||
| if tag_name: | ||
| images_dict[image_id]["tags"].append(tag_name) | ||
|
|
||
| # Convert to list and set tags to None if empty | ||
| images = [] | ||
| for image_data in images_dict.values(): | ||
| if not image_data["tags"]: | ||
| image_data["tags"] = None | ||
| images.append(image_data) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
folder_id type mismatch vs declared types and likely FE contract
This returns folder_id as an integer from SQLite, but FolderId is aliased to str in this module and FE models typically use numbers. Align types to prevent silent schema drift.
Option A (preferred): make FolderId an int (module-wide).
# at top-level type aliases
FolderId = intOption B: if you must keep FolderId=str, cast here:
- images_dict[image_id] = {"id": image_id, "path": path, "folder_id": folder_id, "thumbnailPath": thumbnail_path, "metadata": metadata, "isTagged": bool(is_tagged), "tags": []}
+ images_dict[image_id] = {"id": image_id, "path": path, "folder_id": str(folder_id), "thumbnailPath": thumbnail_path, "metadata": metadata, "isTagged": bool(is_tagged), "tags": []}Run to confirm FE expects number for folder_id:
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts --type=tsx -C2 '\bfolder_id\s*:\s*(number|string)' frontend/src || trueLength of output: 117
🏁 Script executed:
#!/bin/bash
# Search for snake_case usage in frontend
rg -nP -C2 '\bfolder_id\s*:' -g '*.ts' -g '*.tsx' frontend/src || true
# Search for camelCase definitions and their types
rg -nP -C2 '\bfolderId\s*:\s*(number|string)' -g '*.ts' -g '*.tsx' frontend/src || trueLength of output: 3824
Cast folder_id to string to match FE types
In backend/app/database/images.py, wrap folder_id in str() when building the image dict:
- images_dict[image_id] = {
- "id": image_id,
- "path": path,
- "folder_id": folder_id,
- "thumbnailPath": thumbnail_path,
- "metadata": metadata,
- "isTagged": bool(is_tagged),
- "tags": []
- }
+ images_dict[image_id] = {
+ "id": image_id,
+ "path": path,
+ "folder_id": str(folder_id),
+ "thumbnailPath": thumbnail_path,
+ "metadata": metadata,
+ "isTagged": bool(is_tagged),
+ "tags": []
+ }FE’s SyncFolderRequest and FolderDetails interfaces both declare folder_id: string, so casting here avoids type mismatches.
📝 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.
| for image_id, path, folder_id, thumbnail_path, metadata, is_tagged, tag_name in results: | |
| if image_id not in images_dict: | |
| images_dict[image_id] = {"id": image_id, "path": path, "folder_id": folder_id, "thumbnailPath": thumbnail_path, "metadata": metadata, "isTagged": bool(is_tagged), "tags": []} | |
| # Add tag if it exists | |
| if tag_name: | |
| images_dict[image_id]["tags"].append(tag_name) | |
| # Convert to list and set tags to None if empty | |
| images = [] | |
| for image_data in images_dict.values(): | |
| if not image_data["tags"]: | |
| image_data["tags"] = None | |
| images.append(image_data) | |
| for image_id, path, folder_id, thumbnail_path, metadata, is_tagged, tag_name in results: | |
| if image_id not in images_dict: | |
| images_dict[image_id] = { | |
| "id": image_id, | |
| "path": path, | |
| "folder_id": str(folder_id), | |
| "thumbnailPath": thumbnail_path, | |
| "metadata": metadata, | |
| "isTagged": bool(is_tagged), | |
| "tags": [] | |
| } | |
| # Add tag if it exists | |
| if tag_name: | |
| images_dict[image_id]["tags"].append(tag_name) | |
| # Convert to list and set tags to None if empty | |
| images = [] | |
| for image_data in images_dict.values(): | |
| if not image_data["tags"]: | |
| image_data["tags"] = None | |
| images.append(image_data) |
🤖 Prompt for AI Agents
In backend/app/database/images.py around lines 125 to 139, the image dict is
populated with folder_id as-is which is an int on the backend but the frontend
expects a string; change the dict construction to cast folder_id to a string
(wrap folder_id with str()) when assigning the "folder_id" field so the returned
images list matches the FE types.
| # Add tag if it exists | ||
| if tag_name: | ||
| images_dict[image_id]["tags"].append(tag_name) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deduplicate tags to avoid duplicates from joins
If image_classes has duplicate rows or mappings alias the same name, this can add duplicates.
Apply:
- if tag_name:
- images_dict[image_id]["tags"].append(tag_name)
+ if tag_name and tag_name not in images_dict[image_id]["tags"]:
+ images_dict[image_id]["tags"].append(tag_name)📝 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.
| # Add tag if it exists | |
| if tag_name: | |
| images_dict[image_id]["tags"].append(tag_name) | |
| # Add tag if it exists (and avoid duplicates) | |
| - if tag_name: | |
| if tag_name and tag_name not in images_dict[image_id]["tags"]: | |
| images_dict[image_id]["tags"].append(tag_name) |
🤖 Prompt for AI Agents
In backend/app/database/images.py around lines 129 to 132, tag values may be
appended multiple times due to duplicate join rows; change the logic so tags are
deduplicated before adding them — e.g., only append tag_name if it's truthy and
not already present in images_dict[image_id]["tags"], or collect tags into a set
per image (then convert to list) to ensure no duplicates are stored while
preserving existing behavior.
| @@ -1,5 +1,5 @@ | |||
| from pydantic import BaseModel | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce strict response/request schemas (forbid unknown fields).
Prevent silent contract drift by forbidding extra fields across all models.
Apply:
-from pydantic import BaseModel
+from pydantic import BaseModel, ConfigDict
+
+class APIModel(BaseModel):
+ # Pydantic v2
+ model_config = ConfigDict(extra="forbid")
# Request Models
-class RenameClusterRequest(BaseModel):
+class RenameClusterRequest(APIModel):
cluster_name: str
# Response Models
-class RenameClusterData(BaseModel):
+class RenameClusterData(APIModel):
cluster_id: str
cluster_name: str
-class RenameClusterResponse(BaseModel):
+class RenameClusterResponse(APIModel):
success: bool
message: Optional[str] = None
error: Optional[str] = None
data: Optional[RenameClusterData] = None
-class ErrorResponse(BaseModel):
+class ErrorResponse(APIModel):
success: bool = False
message: Optional[str] = None
error: Optional[str] = None
-class ClusterMetadata(BaseModel):
+class ClusterMetadata(APIModel):
cluster_id: str
cluster_name: Optional[str]
face_image_base64: Optional[str]
face_count: int
-class GetClustersData(BaseModel):
+class GetClustersData(APIModel):
clusters: List[ClusterMetadata]
-class GetClustersResponse(BaseModel):
+class GetClustersResponse(APIModel):
success: bool
message: Optional[str] = None
error: Optional[str] = None
data: Optional[GetClustersData] = None
-class ImageInCluster(BaseModel):
+class ImageInCluster(APIModel):
"""Represents an image that contains faces from a specific cluster."""
id: str
path: str
thumbnailPath: Optional[str] = None
metadata: Optional[str] = None
face_id: int
confidence: Optional[float] = None
bbox: Optional[Dict[str, Union[int, float]]] = None
-class GetClusterImagesData(BaseModel):
+class GetClusterImagesData(APIModel):
"""Data model for cluster images response."""
cluster_id: str
cluster_name: Optional[str] = None
images: List[ImageInCluster]
total_images: int
-class GetClusterImagesResponse(BaseModel):
+class GetClusterImagesResponse(APIModel):
"""Response model for getting images in a cluster."""
success: bool
message: Optional[str] = None
error: Optional[str] = None
data: Optional[GetClusterImagesData] = NoneAlso applies to: 5-7, 11-14, 16-20, 23-26, 29-34, 36-38, 40-45, 47-57, 59-66, 68-74
🤖 Prompt for AI Agents
In backend/app/schemas/face_clusters.py (apply to lines 1 and ranges 5-7, 11-14,
16-20, 23-26, 29-34, 36-38, 40-45, 47-57, 59-66, 68-74), each Pydantic model
must forbid unknown fields to prevent silent contract drift; add a Config inner
class to every BaseModel-derived class with extra = "forbid" (or Extra.forbid if
you prefer importing Extra) so any unexpected fields raise validation errors,
ensuring both request and response schemas reject unknown properties.
| ...res, | ||
| successData: res.data?.data, | ||
| errorMessage: res.data?.error || 'Something went wrong', | ||
| errorMessage: res.error ? getErrorMessage(res.error) : undefined, | ||
| successMessage: res.data?.message, | ||
| }; |
There was a problem hiding this comment.
Bug: drops server error strings when HTTP 200 with success=false.
errorMessage only reads res.error (thrown errors). If backend returns { success:false, error }, UI won’t see it.
return {
...res,
successData: res.data?.data,
- errorMessage: res.error ? getErrorMessage(res.error) : undefined,
+ errorMessage: res.error
+ ? getErrorMessage(res.error)
+ : (res.data?.error ?? res.data?.message),
successMessage: res.data?.message,
};📝 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.
| ...res, | |
| successData: res.data?.data, | |
| errorMessage: res.data?.error || 'Something went wrong', | |
| errorMessage: res.error ? getErrorMessage(res.error) : undefined, | |
| successMessage: res.data?.message, | |
| }; | |
| return { | |
| ...res, | |
| successData: res.data?.data, | |
| errorMessage: res.error | |
| ? getErrorMessage(res.error) | |
| : (res.data?.error ?? res.data?.message), | |
| successMessage: res.data?.message, | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/hooks/useQueryExtension.ts around lines 73 to 77, the current
logic only sets errorMessage from res.error (thrown errors) which ignores server
responses that return HTTP 200 with { success:false, error: "..." }; update the
returned shape so errorMessage is set to getErrorMessage(res.error) if present,
otherwise to res.data?.error (or undefined), and only populate successData when
res.data?.success is not false (i.e., set successData = res.data?.data only if
res.data?.success !== false) so server-reported errors surface to the UI and
successful-data isn’t returned on success:false responses.
| const defaultOptions = { | ||
| retry: 2, | ||
| retryDelay: 500, | ||
| }; | ||
|
|
||
| const res = useQuery<TQueryFnData, TError, TQueryFnData, TQueryKey>({ | ||
| ...defaultOptions, | ||
| ...options, | ||
| }); | ||
|
|
||
| return { | ||
| ...res, | ||
| successData: res.data?.data, | ||
| errorMessage: res.data?.error || 'Something went wrong', | ||
| errorMessage: res.error ? getErrorMessage(res.error) : undefined, | ||
| successMessage: res.data?.message, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the same error handling for queries.
Expose backend-provided error/message when the query resolves with success:false.
const res = useQuery<TQueryFnData, TError, TQueryFnData, TQueryKey>({
...defaultOptions,
...options,
});
return {
...res,
successData: res.data?.data,
- errorMessage: res.error ? getErrorMessage(res.error) : undefined,
+ errorMessage: res.error
+ ? getErrorMessage(res.error)
+ : (res.data?.error ?? res.data?.message),
successMessage: res.data?.message,
};📝 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 defaultOptions = { | |
| retry: 2, | |
| retryDelay: 500, | |
| }; | |
| const res = useQuery<TQueryFnData, TError, TQueryFnData, TQueryKey>({ | |
| ...defaultOptions, | |
| ...options, | |
| }); | |
| return { | |
| ...res, | |
| successData: res.data?.data, | |
| errorMessage: res.data?.error || 'Something went wrong', | |
| errorMessage: res.error ? getErrorMessage(res.error) : undefined, | |
| successMessage: res.data?.message, | |
| const defaultOptions = { | |
| retry: 2, | |
| retryDelay: 500, | |
| }; | |
| const res = useQuery<TQueryFnData, TError, TQueryFnData, TQueryKey>({ | |
| ...defaultOptions, | |
| ...options, | |
| }); | |
| return { | |
| ...res, | |
| successData: res.data?.data, | |
| errorMessage: res.error | |
| ? getErrorMessage(res.error) | |
| : (res.data?.error ?? res.data?.message), | |
| successMessage: res.data?.message, | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/hooks/useQueryExtension.ts around lines 92 to 106, the query
return currently only maps res.error and successMessage from res.data.message
but does not expose backend-provided error/message when the API responded with
success:false; update the returned shape so that: set successData to
res.data?.data only when res.data?.success === true, set successMessage to
res.data?.message only when res.data?.success === true, and set errorMessage to
res.error ? getErrorMessage(res.error) : res.data?.success === false ?
res.data?.message : undefined so backend error messages propagate when a
successful HTTP response contains success:false. Ensure existing res.error
handling still takes precedence.
| let slideshowInterval: NodeJS.Timeout | null = null; | ||
|
|
||
| if (isSlideshowActive && totalImages > 1) { | ||
| slideshowInterval = setInterval(() => { | ||
| onNextImage(); | ||
| }, 3000); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix timer type for browser + avoid interval resets when onNextImage identity changes
- NodeJS.Timeout breaks in DOM-only TS configs.
- Interval restarts if parent recreates onNextImage each render.
Apply:
-import { useState, useEffect, useCallback } from 'react';
+import { useState, useEffect, useCallback, useRef } from 'react';
useEffect(() => {
- let slideshowInterval: NodeJS.Timeout | null = null;
+ let slideshowInterval: ReturnType<typeof setInterval> | null = null;
- if (isSlideshowActive && totalImages > 1) {
- slideshowInterval = setInterval(() => {
- onNextImage();
- }, 3000);
- }
+ // keep latest callback without resetting interval
+ const cbRef = { current: onNextImage };
+ if (isSlideshowActive && totalImages > 1) {
+ slideshowInterval = setInterval(() => {
+ cbRef.current();
+ }, 3000);
+ }
@@
- }, [isSlideshowActive, totalImages, onNextImage]);
+ }, [isSlideshowActive, totalImages]);Alternative: keep a separate useRef(onNextImage) + effect to update it; result is the same.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/hooks/useSlideshow.ts around lines 7 to 13, the code uses
NodeJS.Timeout and directly captures onNextImage in the effect which causes type
issues in DOM-only TS configs and unwanted interval resets when onNextImage
identity changes; change the interval type to number | null (or
ReturnType<typeof setInterval>) and manage the callback with a ref: create a
const onNextImageRef = useRef(onNextImage), update onNextImageRef.current in an
effect when onNextImage changes, then create the interval in a separate effect
that depends only on isSlideshowActive and totalImages (not onNextImage) and
calls onNextImageRef.current(), and ensure you clear the interval on cleanup
using window.clearInterval and keep the interval value typed as number | null.
| @@ -1,6 +1,28 @@ | |||
| import { clsx, type ClassValue } from 'clsx'; | |||
| import { twMerge } from 'tailwind-merge'; | |||
| import { AxiosError } from 'axios'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use axios' official type guard for reliability.
Prefer isAxiosError() over checking .isAxiosError to avoid false positives and improve type narrowing.
-import { AxiosError } from 'axios';
+import { AxiosError, isAxiosError } from 'axios';📝 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.
| import { AxiosError } from 'axios'; | |
| import { AxiosError, isAxiosError } from 'axios'; |
🤖 Prompt for AI Agents
In frontend/src/lib/utils.ts around line 3, the code imports AxiosError and
likely checks error.isAxiosError manually; replace that pattern with axios'
official type guard by importing isAxiosError from 'axios' and using if
(isAxiosError(err)) { ... } to reliably narrow types and avoid false positives;
update imports and any error-checking branches to use isAxiosError and adjust
type annotations inside those branches accordingly.
| if ((error as AxiosError).isAxiosError) { | ||
| const axiosErr = error as AxiosError<any>; | ||
| const resData = axiosErr.response?.data; | ||
|
|
||
| // Case 1: Response exists and contains error/message fields | ||
| if (resData && (resData.error || resData.message)) { | ||
| return [resData.error, resData.message].filter(Boolean).join(' - '); | ||
| } | ||
|
|
||
| // Case 2: Response missing error/message, fallback to Axios error details | ||
| return `${axiosErr.code || 'ERROR'}: ${axiosErr.message}`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden Axios error parsing and cover common API shapes.
- Use
isAxiosError(error)for narrowing. - Handle string bodies,
detail, and arrayerrors. - Include HTTP status/statusText in fallback for better context.
- if ((error as AxiosError).isAxiosError) {
- const axiosErr = error as AxiosError<any>;
- const resData = axiosErr.response?.data;
-
- // Case 1: Response exists and contains error/message fields
- if (resData && (resData.error || resData.message)) {
- return [resData.error, resData.message].filter(Boolean).join(' - ');
- }
-
- // Case 2: Response missing error/message, fallback to Axios error details
- return `${axiosErr.code || 'ERROR'}: ${axiosErr.message}`;
- }
+ if (isAxiosError(error)) {
+ const axiosErr = error as AxiosError<unknown>;
+ const data = axiosErr.response?.data as any;
+
+ if (typeof data === 'string') return data;
+ if (data && typeof data === 'object') {
+ const msg = [
+ typeof data.error === 'string' ? data.error : undefined,
+ typeof data.message === 'string' ? data.message : undefined,
+ typeof data.detail === 'string' ? data.detail : undefined,
+ Array.isArray(data.errors) ? data.errors.filter(Boolean).join(', ') : undefined,
+ ]
+ .filter(Boolean)
+ .join(' - ');
+ if (msg) return msg;
+ }
+
+ const status = axiosErr.response?.status;
+ const statusText = axiosErr.response?.statusText;
+ const header = [status, statusText, axiosErr.code || 'ERROR'].filter(Boolean).join(' ');
+ return [header, axiosErr.message].filter(Boolean).join(': ');
+ }📝 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.
| if ((error as AxiosError).isAxiosError) { | |
| const axiosErr = error as AxiosError<any>; | |
| const resData = axiosErr.response?.data; | |
| // Case 1: Response exists and contains error/message fields | |
| if (resData && (resData.error || resData.message)) { | |
| return [resData.error, resData.message].filter(Boolean).join(' - '); | |
| } | |
| // Case 2: Response missing error/message, fallback to Axios error details | |
| return `${axiosErr.code || 'ERROR'}: ${axiosErr.message}`; | |
| } | |
| if (isAxiosError(error)) { | |
| const axiosErr = error as AxiosError<unknown>; | |
| const data = axiosErr.response?.data as any; | |
| if (typeof data === 'string') return data; | |
| if (data && typeof data === 'object') { | |
| const msg = [ | |
| typeof data.error === 'string' ? data.error : undefined, | |
| typeof data.message === 'string' ? data.message : undefined, | |
| typeof data.detail === 'string' ? data.detail : undefined, | |
| Array.isArray(data.errors) ? data.errors.filter(Boolean).join(', ') : undefined, | |
| ] | |
| .filter(Boolean) | |
| .join(' - '); | |
| if (msg) return msg; | |
| } | |
| const status = axiosErr.response?.status; | |
| const statusText = axiosErr.response?.statusText; | |
| const header = [status, statusText, axiosErr.code || 'ERROR'].filter(Boolean).join(' '); | |
| return [header, axiosErr.message].filter(Boolean).join(': '); | |
| } |
🤖 Prompt for AI Agents
In frontend/src/lib/utils.ts around lines 13 to 24, the Axios error handling is
too narrow; replace the type-assertion approach with axios.isAxiosError(error)
to narrow, then extract response payload defensively: if response.data is a
string use it directly; if it's an object prefer fields in order (error,
message, detail) and if there is an errors array join its items; for existing
error/message combine non-empty values with ' - '; as a fallback include HTTP
context (response.status and response.statusText) and axiosErr.code plus
axiosErr.message to form a clear fallback string. Ensure all accesses are
guarded (optional chaining) and always return a readable string.
| # Restart sync microservice watcher after processing images | ||
| API_util_restart_sync_microservice_watcher() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don't ignore restart outcome; log or retry on failure.
The external call returns a bool. Swallowing failures hinders operability and support.
- # Restart sync microservice watcher after processing images
- API_util_restart_sync_microservice_watcher()
+ # Restart sync microservice watcher after processing images
+ if not API_util_restart_sync_microservice_watcher():
+ # TODO: consider retry/backoff
+ print("Warning: Failed to restart sync microservice watcher")📝 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.
| # Restart sync microservice watcher after processing images | |
| API_util_restart_sync_microservice_watcher() | |
| # Restart sync microservice watcher after processing images | |
| if not API_util_restart_sync_microservice_watcher(): | |
| # TODO: consider retry/backoff | |
| print("Warning: Failed to restart sync microservice watcher") |
🤖 Prompt for AI Agents
In backend/app/routes/folders.py around lines 70 to 72, the call to
API_util_restart_sync_microservice_watcher() ignores its boolean return value;
capture the returned bool, and if it indicates failure, log an error with
context (including folder id or request info) and either retry (e.g., limited
attempts with small backoff) or raise/propagate an exception so the caller can
handle it; ensure any retry loop has a max attempts and that all failures are
recorded via the existing logger before returning an error response.
| # Restart sync microservice watcher after processing images | ||
| API_util_restart_sync_microservice_watcher() | ||
| except Exception as e: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same as above: handle restart failures during sync.
Mirror the handling added to post-add to avoid silent failures.
- # Restart sync microservice watcher after processing images
- API_util_restart_sync_microservice_watcher()
+ # Restart sync microservice watcher after processing images
+ if not API_util_restart_sync_microservice_watcher():
+ print("Warning: Failed to restart sync microservice watcher")📝 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.
| # Restart sync microservice watcher after processing images | |
| API_util_restart_sync_microservice_watcher() | |
| except Exception as e: | |
| # Restart sync microservice watcher after processing images | |
| if not API_util_restart_sync_microservice_watcher(): | |
| print("Warning: Failed to restart sync microservice watcher") | |
| except Exception as e: |
🤖 Prompt for AI Agents
In backend/app/routes/folders.py around lines 115 to 117, the call to
API_util_restart_sync_microservice_watcher() is unprotected and may fail
silently; wrap this call in a try/except that mirrors the handling used in the
post-add flow: catch Exception as err, log the error with context (including
err) and any actionable message, and avoid crashing the handler (return/continue
gracefully) so restart failures are visible but do not break overall processing.
| class ImageData(BaseModel): | ||
| id: str | ||
| path: str | ||
| folder_id: str | ||
| thumbnailPath: str | ||
| metadata: str | ||
| isTagged: bool | ||
| tags: Optional[List[str]] = None |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align field types with DB/API contract.
id/folder_id look numeric in SQLite. Typing them as str may coerce ints to strings and change clients’ expectations.
class ImageData(BaseModel):
- id: str
+ id: int
path: str
- folder_id: str
+ folder_id: int | None = None
thumbnailPath: str
- metadata: str
+ metadata: str # consider dict if JSON
isTagged: bool
tags: Optional[List[str]] = NoneConfirm DB schema and OpenAPI; adjust types accordingly.
🏁 Script executed:
#!/bin/bash
set -e
echo "Searching for SQLAlchemy model definitions of Image"
rg -nP --type py "class\s+Image"
echo "Searching for folder_id usages"
rg -n "folder_id" -g "*.py"
echo "Searching for table definition"
rg -nP "Table\(\s*['\"]images['\"]" -g "*.py"Length of output: 27865
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Images DB CREATE TABLE ==="
rg -nP -C3 "CREATE TABLE" -g "backend/app/database/images.py" || true
echo -e "\n=== Folders DB CREATE TABLE ==="
rg -nP -C3 "CREATE TABLE" -g "backend/app/database/folders.py" || true
echo -e "\n=== Images DB Column Types ==="
rg -nP -C3 "id\s+INTEGER" -g "backend/app/database/images.py" || true
rg -nP -C3 "folder_id\s+INTEGER" -g "backend/app/database/images.py" || true
echo -e "\n=== Pydantic Model: ImageData ==="
sed -n '1,50p' backend/app/routes/images.pyLength of output: 4797
Align folder_id type with DB schema
Change the folder_id field on ImageData (backend/app/routes/images.py) from str to int; leave id: str (DB column is TEXT). Optional: convert metadata to a dict if it holds JSON.
🤖 Prompt for AI Agents
In backend/app/routes/images.py around lines 11 to 18, the ImageData Pydantic
model declares folder_id as str but the DB schema uses an integer; change
folder_id: str to folder_id: int (keep id: str as-is). If metadata stores JSON,
change metadata: str to metadata: dict (or Optional[dict]) and add the
appropriate typing imports (Dict or dict/Optional/List as needed); ensure any
usages/serializations of metadata elsewhere are updated to handle a dict.
| db_create_albums_table() | ||
| db_create_album_images_table() | ||
| db_create_metadata_table() | ||
| microservice_util_start_sync_service() |
There was a problem hiding this comment.
Don’t block startup; gate and offload the microservice bootstrap
Starting the sync microservice (venv create/pip install/spawn) in the lifespan blocks the event loop and is risky in prod. Gate via env and run in a thread.
+import asyncio
from contextlib import asynccontextmanager- microservice_util_start_sync_service()
+ if os.getenv("START_SYNC_MICROSERVICE", "false").lower() == "true":
+ await asyncio.to_thread(microservice_util_start_sync_service)Also consider stopping the child process on shutdown; return and store a handle from the util.
Also applies to: 12-12
🤖 Prompt for AI Agents
In backend/main.py around lines 45 (and also at 12), the call
microservice_util_start_sync_service() is invoked during lifespan which blocks
the event loop; gate startup behind an environment flag and offload the heavy
work to a background thread or executor instead of running synchronously in the
lifespan handler, i.e. check an env var (e.g. START_SYNC_MICROSERVICE) before
scheduling the bootstrap; modify microservice_util_start_sync_service to return
a handle/object representing the spawned child (or thread/future) so the
lifespan shutdown can stop/terminate the child cleanly and await thread
termination, and ensure any blocking operations (venv creation/pip
install/process spawn) run in the thread/executor to keep the event loop
responsive.
| export interface SyncFolderRequest { | ||
| folder_path: string; | ||
| folder_id: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused type: implement syncFolder API
You define SyncFolderRequest but don’t expose a function. Add it for completeness.
export interface SyncFolderRequest {
folder_path: string;
folder_id: string;
}
@@
export const deleteFolders = async (
request: DeleteFoldersRequest,
): Promise<APIResponse> => {
const response = await apiClient.delete<APIResponse>(
foldersEndpoints.deleteFolders,
{ data: request },
);
return response.data;
};
+
+export const syncFolder = async (
+ request: SyncFolderRequest,
+): Promise<APIResponse> => {
+ const response = await apiClient.post<APIResponse>(
+ foldersEndpoints.syncFolder,
+ request,
+ );
+ return response.data;
+};📝 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.
| export interface SyncFolderRequest { | |
| folder_path: string; | |
| folder_id: string; | |
| } | |
| export interface SyncFolderRequest { | |
| folder_path: string; | |
| folder_id: string; | |
| } | |
| export const deleteFolders = async ( | |
| request: DeleteFoldersRequest, | |
| ): Promise<APIResponse> => { | |
| const response = await apiClient.delete<APIResponse>( | |
| foldersEndpoints.deleteFolders, | |
| { data: request }, | |
| ); | |
| return response.data; | |
| }; | |
| export const syncFolder = async ( | |
| request: SyncFolderRequest, | |
| ): Promise<APIResponse> => { | |
| const response = await apiClient.post<APIResponse>( | |
| foldersEndpoints.syncFolder, | |
| request, | |
| ); | |
| return response.data; | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/api/api-functions/folders.ts around lines 20 to 23, you declared
the SyncFolderRequest type but did not expose a syncFolder API function; add an
exported async function (e.g., export async function syncFolder(req:
SyncFolderRequest): Promise<...>) that calls the appropriate HTTP endpoint (POST
or PUT to /folders/sync or similar), sends the request body with folder_path and
folder_id, handles HTTP errors and returns typed response data; ensure the
function is exported from the module and update any index barrel exports if
needed.
| export const selectFolderHierarchy = createSelector( | ||
| [selectAllFolders], | ||
| (folders) => { | ||
| const buildHierarchy = ( | ||
| parentId: string | null = null, | ||
| ): FolderDetails[] => { | ||
| return folders | ||
| .filter((folder) => folder.parent_folder_id === parentId) | ||
| .map((folder) => ({ | ||
| ...folder, | ||
| children: buildHierarchy(folder.folder_id), | ||
| })); | ||
| }; | ||
|
|
||
| return buildHierarchy(); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Type mismatch and O(n^2) recursion in hierarchy builder
You’re returning objects with children while typing as FolderDetails[]. Also, filtering on every recursion leads to O(n^2). Build an adjacency map once and return a typed node tree. Add simple cycle-guard.
export const selectFolderHierarchy = createSelector(
[selectAllFolders],
(folders) => {
- const buildHierarchy = (
- parentId: string | null = null,
- ): FolderDetails[] => {
- return folders
- .filter((folder) => folder.parent_folder_id === parentId)
- .map((folder) => ({
- ...folder,
- children: buildHierarchy(folder.folder_id),
- }));
- };
-
- return buildHierarchy();
+ type FolderNode = FolderDetails & { children: FolderNode[] };
+ const childrenByParent = new Map<string | null, FolderDetails[]>();
+ for (const f of folders) {
+ const key = f.parent_folder_id ?? null;
+ const list = childrenByParent.get(key);
+ if (list) list.push(f);
+ else childrenByParent.set(key, [f]);
+ }
+ const visited = new Set<string>();
+ const build = (parentId: string | null = null): FolderNode[] =>
+ (childrenByParent.get(parentId) ?? []).map((f) => {
+ if (visited.has(f.folder_id)) {
+ // cycle guard
+ return { ...f, children: [] };
+ }
+ visited.add(f.folder_id);
+ return { ...f, children: build(f.folder_id) };
+ });
+ return build(null);
},
);If you want stronger typing across the app, extract FolderNode to '@/types/Folder' as a dedicated interface.
📝 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.
| export const selectFolderHierarchy = createSelector( | |
| [selectAllFolders], | |
| (folders) => { | |
| const buildHierarchy = ( | |
| parentId: string | null = null, | |
| ): FolderDetails[] => { | |
| return folders | |
| .filter((folder) => folder.parent_folder_id === parentId) | |
| .map((folder) => ({ | |
| ...folder, | |
| children: buildHierarchy(folder.folder_id), | |
| })); | |
| }; | |
| return buildHierarchy(); | |
| }, | |
| ); | |
| export const selectFolderHierarchy = createSelector( | |
| [selectAllFolders], | |
| (folders) => { | |
| // local node type with children | |
| type FolderNode = FolderDetails & { children: FolderNode[] }; | |
| // build adjacency map: parentId -> list of children | |
| const childrenByParent = new Map<string | null, FolderDetails[]>(); | |
| for (const f of folders) { | |
| const key = f.parent_folder_id ?? null; | |
| const list = childrenByParent.get(key); | |
| if (list) list.push(f); | |
| else childrenByParent.set(key, [f]); | |
| } | |
| // cycle guard set | |
| const visited = new Set<string>(); | |
| // recursive builder using the adjacency map | |
| const build = (parentId: string | null = null): FolderNode[] => | |
| (childrenByParent.get(parentId) ?? []).map((f) => { | |
| if (visited.has(f.folder_id)) { | |
| // cycle detected: stop here | |
| return { ...f, children: [] }; | |
| } | |
| visited.add(f.folder_id); | |
| return { ...f, children: build(f.folder_id) }; | |
| }); | |
| return build(null); | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In frontend/src/features/folderSelectors.ts around lines 50 to 66, the current
recursive builder returns objects with children but is typed as FolderDetails[]
and does repeated filtering causing O(n^2) behavior; replace it by building a
single adjacency map (id -> children array) once from folders, implement a typed
FolderNode (exported to '@/types/Folder' if you want app-wide typing) and have
the selector return FolderNode[] (or FolderNode | null roots) instead of
FolderDetails[], and add a simple cycle guard (visited Set of ids) in the
recursion to prevent infinite loops and skip nodes seen twice; update types and
usages accordingly.
| setImages(state, action: PayloadAction<Image[]>) { | ||
| state.images = action.payload; | ||
| state.totalImages = action.payload.length; | ||
| state.error = null; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clamp/reset currentViewIndex when replacing images
Replacing the array can leave a stale index. Clamp to range or reset to -1 to avoid invalid state.
setImages(state, action: PayloadAction<Image[]>) {
state.images = action.payload;
state.totalImages = action.payload.length;
state.error = null;
+ // Keep viewer valid
+ const len = state.images.length;
+ if (len === 0) {
+ state.currentViewIndex = -1;
+ } else if (state.currentViewIndex >= len) {
+ state.currentViewIndex = len - 1;
+ }
},📝 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.
| setImages(state, action: PayloadAction<Image[]>) { | |
| state.images = action.payload; | |
| state.totalImages = action.payload.length; | |
| state.error = null; | |
| }, | |
| setImages(state, action: PayloadAction<Image[]>) { | |
| state.images = action.payload; | |
| state.totalImages = action.payload.length; | |
| state.error = null; | |
| // Keep viewer valid | |
| const len = state.images.length; | |
| if (len === 0) { | |
| state.currentViewIndex = -1; | |
| } else if (state.currentViewIndex >= len) { | |
| state.currentViewIndex = len - 1; | |
| } | |
| }, |
🤖 Prompt for AI Agents
In frontend/src/features/imageSlice.ts around lines 22 to 26, replacing
state.images without updating state.currentViewIndex can leave a
stale/out-of-range index; modify setImages so after assigning state.images and
state.totalImages you clamp state.currentViewIndex into the valid range (e.g. if
payload.length === 0 set to -1, otherwise set to
Math.min(Math.max(state.currentViewIndex ?? -1, -1), payload.length - 1)) or
explicitly reset to -1; ensure state.error is still cleared.
| removeImage(state, action: PayloadAction<string>) { | ||
| const imageId = action.payload; | ||
| const imageIndex = state.images.findIndex( | ||
| (image) => image.id === imageId, | ||
| ); | ||
| if (imageIndex !== -1) { | ||
| state.images.splice(imageIndex, 1); | ||
| state.totalImages = state.images.length; | ||
|
|
||
| // Adjust currentViewIndex if necessary | ||
| if ( | ||
| state.currentViewIndex >= imageIndex && | ||
| state.currentViewIndex > 0 | ||
| ) { | ||
| state.currentViewIndex -= 1; | ||
| } else if (state.currentViewIndex >= state.images.length) { | ||
| state.currentViewIndex = state.images.length - 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make index adjustment robust after removal
Cover all cases (removed before current, removed last, empty array).
- if (imageIndex !== -1) {
- state.images.splice(imageIndex, 1);
- state.totalImages = state.images.length;
-
- // Adjust currentViewIndex if necessary
- if (
- state.currentViewIndex >= imageIndex &&
- state.currentViewIndex > 0
- ) {
- state.currentViewIndex -= 1;
- } else if (state.currentViewIndex >= state.images.length) {
- state.currentViewIndex = state.images.length - 1;
- }
- }
+ if (imageIndex !== -1) {
+ state.images.splice(imageIndex, 1);
+ state.totalImages = state.images.length;
+ const len = state.images.length;
+ if (len === 0) {
+ state.currentViewIndex = -1;
+ } else if (state.currentViewIndex > imageIndex) {
+ state.currentViewIndex -= 1;
+ } else if (state.currentViewIndex >= len) {
+ state.currentViewIndex = len - 1;
+ }
+ }📝 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.
| removeImage(state, action: PayloadAction<string>) { | |
| const imageId = action.payload; | |
| const imageIndex = state.images.findIndex( | |
| (image) => image.id === imageId, | |
| ); | |
| if (imageIndex !== -1) { | |
| state.images.splice(imageIndex, 1); | |
| state.totalImages = state.images.length; | |
| // Adjust currentViewIndex if necessary | |
| if ( | |
| state.currentViewIndex >= imageIndex && | |
| state.currentViewIndex > 0 | |
| ) { | |
| state.currentViewIndex -= 1; | |
| } else if (state.currentViewIndex >= state.images.length) { | |
| state.currentViewIndex = state.images.length - 1; | |
| } | |
| } | |
| removeImage(state, action: PayloadAction<string>) { | |
| const imageId = action.payload; | |
| const imageIndex = state.images.findIndex( | |
| (image) => image.id === imageId, | |
| ); | |
| if (imageIndex !== -1) { | |
| state.images.splice(imageIndex, 1); | |
| state.totalImages = state.images.length; | |
| const len = state.images.length; | |
| if (len === 0) { | |
| state.currentViewIndex = -1; | |
| } else if (state.currentViewIndex > imageIndex) { | |
| state.currentViewIndex -= 1; | |
| } else if (state.currentViewIndex >= len) { | |
| state.currentViewIndex = len - 1; | |
| } | |
| } |
🤖 Prompt for AI Agents
In frontend/src/features/imageSlice.ts around lines 64 to 82, the
currentViewIndex adjustment after removing an image is incomplete and can
produce out-of-range or incorrect indices (especially when removing the last
item, an item before the current index, or when the array becomes empty); update
the logic to (1) decrement currentViewIndex by 1 only when the removed index is
strictly less than the currentViewIndex, (2) if the array becomes empty set
currentViewIndex to 0, and (3) otherwise clamp currentViewIndex to the range [0,
state.images.length - 1] to ensure it never becomes negative or exceeds the last
index. Ensure these checks run after splicing so state.images.length is the
post-removal length.
| import { fetchAllImages } from '@/api/api-functions'; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify import path for fetchAllImages.
Ensure @/api/api-functions re-exports fetchAllImages; otherwise import from @/api/api-functions/images.
🏁 Script executed:
#!/bin/bash
rg -n "export\\s+\\{[^}]*fetchAllImages" frontend/src/api/api-functions/index.ts || true
fd -n images.ts frontend/src/api/api-functions
rg -n "export const fetchAllImages" frontend/src/api/api-functionsLength of output: 545
Fix import for fetchAllImages
frontend/src/pages/AITagging/AITagging.tsx currently does:
import { fetchAllImages } from '@/api/api-functions';—but frontend/src/api/api-functions/index.ts does not re-export fetchAllImages. Either:
- Change the import to pull directly from the defining module:
import { fetchAllImages } from '@/api/api-functions/images';
- Or add a re-export in
frontend/src/api/api-functions/index.ts:export { fetchAllImages } from './images';
🤖 Prompt for AI Agents
In frontend/src/pages/AITagging/AITagging.tsx around lines 14 to 15, the file
imports fetchAllImages from '@/api/api-functions' but that index.ts does not
re-export fetchAllImages; fix by either changing the import to point directly to
the defining module '@/api/api-functions/images' or by adding a re-export for
fetchAllImages in frontend/src/api/api-functions/index.ts so the named import
resolves correctly.
| if (imagesLoading) { | ||
| dispatch(showLoader('Loading AI tagging data')); | ||
| } else if (imagesError) { | ||
| dispatch(hideLoader()); | ||
| } else if (imagesSuccess) { | ||
| const images = imagesData?.data as Image[]; | ||
| dispatch(setImages(images)); | ||
| dispatch(hideLoader()); | ||
| } | ||
| }, [imagesData, imagesSuccess, imagesError, imagesLoading, dispatch]); | ||
|
|
There was a problem hiding this comment.
Guard undefined API data to avoid runtime crash in reducer.
imagesData?.data as Image[] can be undefined; setImages reads .length, causing a TypeError.
- useEffect(() => {
+ useEffect(() => {
if (imagesLoading) {
dispatch(showLoader('Loading AI tagging data'));
} else if (imagesError) {
dispatch(hideLoader());
} else if (imagesSuccess) {
- const images = imagesData?.data as Image[];
- dispatch(setImages(images));
+ const images = Array.isArray(imagesData?.data) ? (imagesData!.data as Image[]) : [];
+ dispatch(setImages(images));
dispatch(hideLoader());
}
}, [imagesData, imagesSuccess, imagesError, imagesLoading, dispatch]);Alternatively, destructure successData from usePictoQuery and use that directly.
📝 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.
| useEffect(() => { | |
| if (imagesLoading) { | |
| dispatch(showLoader('Loading AI tagging data')); | |
| } else if (imagesError) { | |
| dispatch(hideLoader()); | |
| } else if (imagesSuccess) { | |
| const images = imagesData?.data as Image[]; | |
| dispatch(setImages(images)); | |
| dispatch(hideLoader()); | |
| } | |
| }, [imagesData, imagesSuccess, imagesError, imagesLoading, dispatch]); | |
| useEffect(() => { | |
| if (imagesLoading) { | |
| dispatch(showLoader('Loading AI tagging data')); | |
| } else if (imagesError) { | |
| dispatch(hideLoader()); | |
| } else if (imagesSuccess) { | |
| const images = Array.isArray(imagesData?.data) ? (imagesData!.data as Image[]) : []; | |
| dispatch(setImages(images)); | |
| dispatch(hideLoader()); | |
| } | |
| }, [imagesData, imagesSuccess, imagesError, imagesLoading, dispatch]); |
🤖 Prompt for AI Agents
In frontend/src/pages/AITagging/AITagging.tsx around lines 31 to 41, the code
casts imagesData?.data to Image[] and passes it to setImages even when that
value can be undefined which makes the reducer crash when it reads .length;
change the logic to guard the API result by deriving a safe array (e.g. const
images = (imagesData?.data ?? []) as Image[]) or use the query hook's
successData directly, and only call dispatch(setImages(images)) with that
guaranteed array (or skip dispatch when undefined) so the reducer never receives
an undefined value.
| data-slot="sidebar-gap" | ||
| className={cn( | ||
| 'relative w-(--sidebar-width) bg-transparent transition-[width] duration-200 ease-linear', | ||
| 'group-data-[collapsible=offcanvas]:w-0', | ||
| 'group-data-[side=right]:rotate-180', | ||
| variant === 'floating' || variant === 'inset' | ||
| ? 'group-data-[collapsible=icon]:w-[calc(var(--sidebar-width-icon)+(--spacing(4)))]' | ||
| : 'group-data-[collapsible=icon]:w-(--sidebar-width-icon)', | ||
| )} | ||
| /> |
There was a problem hiding this comment.
Multiple invalid classes and theme() usage.
- w-(--sidebar-width) → w-[var(--sidebar-width)]
- Use Tailwind theme() only inside [].
- Avoid custom (--spacing(4)); use theme(spacing.4) or a fixed calc.
- 'relative w-(--sidebar-width) bg-transparent transition-[width] duration-200 ease-linear',
+ 'relative w-[var(--sidebar-width)] bg-transparent transition-[width] duration-200 ease-linear',
- ? 'group-data-[collapsible=icon]:w-[calc(var(--sidebar-width-icon)+(--spacing(4)))]'
+ ? 'group-data-[collapsible=icon]:w-[calc(var(--sidebar-width-icon)+theme(spacing.4))]'Ensure your Tailwind config supports arbitrary values with theme() in arbitrary properties (Tailwind ≥3.3).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/ui/sidebar.tsx around lines 217 to 226, several
Tailwind arbitrary-value classes are invalid: replace w-(--sidebar-width) with
w-[var(--sidebar-width)] and w-(--sidebar-width-icon) with
w-[var(--sidebar-width-icon)]; for the collapsed-icon width avoid using
(--spacing(4)) — use an arbitrary calc with theme() inside the brackets like
group-data-[collapsible=icon]:w-[calc(var(--sidebar-width-icon)+theme('spacing.4'))]
(or substitute a fixed px/rem value) and ensure your Tailwind config supports
theme() in arbitrary properties (Tailwind ≥3.3).
| data-slot="sidebar-container" | ||
| className={cn( | ||
| 'fixed inset-y-0 z-10 hidden h-svh w-(--sidebar-width) transition-[left,right,width] duration-200 ease-linear md:flex', | ||
| side === 'left' | ||
| ? 'left-0 group-data-[collapsible=offcanvas]:left-[calc(var(--sidebar-width)*-1)]' | ||
| : 'right-0 group-data-[collapsible=offcanvas]:right-[calc(var(--sidebar-width)*-1)]', | ||
| // Adjust the padding for floating and inset variants. | ||
| variant === 'floating' || variant === 'inset' | ||
| ? 'p-2 group-data-[collapsible=icon]:w-[calc(var(--sidebar-width-icon)+(--spacing(4))+2px)]' | ||
| : 'group-data-[collapsible=icon]:w-(--sidebar-width-icon) group-data-[side=left]:border-r group-data-[side=right]:border-l', | ||
| className, | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
More invalid classes and missing border variants.
- w-(--sidebar-width) → w-[var(--sidebar-width)]
- Consider explicit border color var for side borders.
- 'fixed inset-y-0 z-10 hidden h-svh w-(--sidebar-width) transition-[left,right,width] duration-200 ease-linear md:flex',
+ 'fixed inset-y-0 z-10 hidden h-svh w-[var(--sidebar-width)] transition-[left,right,width] duration-200 ease-linear md:flex',
- : 'group-data-[collapsible=icon]:w-(--sidebar-width-icon) group-data-[side=left]:border-r group-data-[side=right]:border-l',
+ : 'group-data-[collapsible=icon]:w-[var(--sidebar-width-icon)] group-data-[side=left]:border-r group-data-[side=right]:border-l',📝 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.
| data-slot="sidebar-container" | |
| className={cn( | |
| 'fixed inset-y-0 z-10 hidden h-svh w-(--sidebar-width) transition-[left,right,width] duration-200 ease-linear md:flex', | |
| side === 'left' | |
| ? 'left-0 group-data-[collapsible=offcanvas]:left-[calc(var(--sidebar-width)*-1)]' | |
| : 'right-0 group-data-[collapsible=offcanvas]:right-[calc(var(--sidebar-width)*-1)]', | |
| // Adjust the padding for floating and inset variants. | |
| variant === 'floating' || variant === 'inset' | |
| ? 'p-2 group-data-[collapsible=icon]:w-[calc(var(--sidebar-width-icon)+(--spacing(4))+2px)]' | |
| : 'group-data-[collapsible=icon]:w-(--sidebar-width-icon) group-data-[side=left]:border-r group-data-[side=right]:border-l', | |
| className, | |
| )} | |
| data-slot="sidebar-container" | |
| className={cn( | |
| 'fixed inset-y-0 z-10 hidden h-svh w-[var(--sidebar-width)] transition-[left,right,width] duration-200 ease-linear md:flex', | |
| side === 'left' | |
| ? 'left-0 group-data-[collapsible=offcanvas]:left-[calc(var(--sidebar-width)*-1)]' | |
| : 'right-0 group-data-[collapsible=offcanvas]:right-[calc(var(--sidebar-width)*-1)]', | |
| // Adjust the padding for floating and inset variants. | |
| variant === 'floating' || variant === 'inset' | |
| ? 'p-2 group-data-[collapsible=icon]:w-[calc(var(--sidebar-width-icon)+(--spacing(4))+2px)]' | |
| : 'group-data-[collapsible=icon]:w-[var(--sidebar-width-icon)] group-data-[side=left]:border-r group-data-[side=right]:border-l', | |
| className, | |
| )} |
🤖 Prompt for AI Agents
In frontend/src/components/ui/sidebar.tsx around lines 228 to 239, replace
invalid Tailwind arbitrary-value syntax and add explicit border color variants:
change w-(--sidebar-width) to w-[var(--sidebar-width)], change
group-data-[collapsible=icon]:w-(--sidebar-width-icon) to
group-data-[collapsible=icon]:w-[var(--sidebar-width-icon)], and append explicit
border color classes to the side border variants (e.g.
group-data-[side=left]:border-r border-[var(--border-color)] and
group-data-[side=right]:border-l border-[var(--border-color)] or use the
project's border token like border-border) so the classes are valid and the side
borders have a defined color.
| 'hover:after:bg-sidebar-border absolute inset-y-0 z-20 hidden w-4 -translate-x-1/2 transition-all ease-linear group-data-[side=left]:-right-4 group-data-[side=right]:left-0 after:absolute after:inset-y-0 after:left-1/2 after:w-[2px] sm:flex', | ||
| 'in-data-[side=left]:cursor-w-resize in-data-[side=right]:cursor-e-resize', | ||
| '[[data-side=left][data-state=collapsed]_&]:cursor-e-resize [[data-side=right][data-state=collapsed]_&]:cursor-w-resize', | ||
| 'hover:group-data-[collapsible=offcanvas]:bg-sidebar group-data-[collapsible=offcanvas]:translate-x-0 group-data-[collapsible=offcanvas]:after:left-full', | ||
| '[[data-side=left][data-collapsible=offcanvas]_&]:-right-2', | ||
| '[[data-side=right][data-collapsible=offcanvas]_&]:-left-2', | ||
| className, | ||
| )} |
There was a problem hiding this comment.
Non-standard variant selectors: in-data[...] and complex ancestor selectors likely won’t compile.
Tailwind supports arbitrary variants like data-[attr=value]: but not in-data[...] or [[..._&]. Simplify to supported variants or move to CSS.
Example rewrite:
- 'in-data-[side=left]:cursor-w-resize in-data-[side=right]:cursor-e-resize',
- '[[data-side=left][data-state=collapsed]_&]:cursor-e-resize [[data-side=right][data-state=collapsed]_&]:cursor-w-resize',
+ 'data-[side=left]:cursor-w-resize data-[side=right]:cursor-e-resize',
+ 'group-data-[side=left]:data-[state=collapsed]:cursor-e-resize group-data-[side=right]:data-[state=collapsed]:cursor-w-resize',If needed, extract these to a CSS file with :has() support and fallbacks.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/ui/sidebar.tsx around lines 292 to 299, the
classnames use unsupported variant syntaxes like in-data[...] and complex
ancestor selectors ([[..._&]) which Tailwind won't compile; replace these with
supported Tailwind arbitrary variants (e.g., data-[side=left]:...,
data-[state=collapsed]:..., group-data-[...] or peer-data-[...] patterns) or
move the complex state/ancestor rules into a separate CSS file (using :has() if
available) with appropriate fallbacks; ensure you rewrite each problematic
selector to an equivalent supported variant or CSS rule and update the component
to use group/peer attributes or data- attributes consistently.
| const sidebarMenuButtonVariants = cva( | ||
| 'peer/menu-button flex w-full items-center gap-2 overflow-hidden rounded-md p-2 text-left text-sm outline-hidden ring-sidebar-ring transition-[width,height,padding] hover:bg-sidebar-accent hover:text-sidebar-accent-foreground focus-visible:ring-2 active:bg-sidebar-accent active:text-sidebar-accent-foreground disabled:pointer-events-none disabled:opacity-50 group-has-data-[sidebar=menu-action]/menu-item:pr-8 aria-disabled:pointer-events-none aria-disabled:opacity-50 data-[active=true]:bg-sidebar-accent data-[active=true]:font-medium data-[active=true]:text-sidebar-accent-foreground data-[state=open]:hover:bg-sidebar-accent data-[state=open]:hover:text-sidebar-accent-foreground group-data-[collapsible=icon]:size-8! group-data-[collapsible=icon]:p-2! [&>span:last-child]:truncate [&>svg]:size-4 [&>svg]:shrink-0', | ||
| { | ||
| variants: { | ||
| variant: { | ||
| default: 'hover:bg-sidebar-accent hover:text-sidebar-accent-foreground', | ||
| outline: | ||
| 'bg-background shadow-[0_0_0_1px_hsl(var(--sidebar-border))] hover:bg-sidebar-accent hover:text-sidebar-accent-foreground hover:shadow-[0_0_0_1px_hsl(var(--sidebar-accent))]', | ||
| }, | ||
| size: { | ||
| default: 'h-8 text-sm', | ||
| sm: 'h-7 text-xs', | ||
| lg: 'h-12 text-sm group-data-[collapsible=icon]:p-0!', | ||
| }, | ||
| }, | ||
| defaultVariants: { | ||
| variant: 'default', | ||
| size: 'default', | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Invalid Tailwind important modifier usage and non-standard utilities.
- Use !size-8 (prefix) not size-8!
- outline-hidden is not a Tailwind utility; use outline-none.
- lg: 'h-12 text-sm group-data-[collapsible=icon]:p-0!',
+ lg: 'h-12 text-sm !group-data-[collapsible=icon]:p-0',And where used:
- '... group-data-[collapsible=icon]:size-8! group-data-[collapsible=icon]:p-2! ...'
+ '... !group-data-[collapsible=icon]:size-8 !group-data-[collapsible=icon]:p-2 ...'Also replace occurrences of outline-hidden with outline-none.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/ui/sidebar.tsx around lines 474 to 494, the class
string uses non-standard Tailwind utilities and incorrect important modifier
placement; replace outline-hidden with outline-none, and move the trailing "!"
important modifier to the prefix form for the size utility (e.g. change
group-data-[collapsible=icon]:size-8! to group-data-[collapsible=icon]:!size-8);
also scan and fix any other occurrences of trailing "!" on utilities (e.g.
[&>svg]:size-4 -> [&>svg]:!size-4) to the correct !utility prefix form.
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
backend/app/utils/images.py (1)
204-216: Raising default thumbnail size to 600×600 has perf/disk impact; make it configurable and optimize save.
- 9× more pixels than 200×200 increases CPU, I/O, and storage.
- Add config-driven default, correct EXIF orientation, set resample, and compress JPEG.
Apply this focused change to the signature to allow a config fallback:
-def image_util_generate_thumbnail( - image_path: str, thumbnail_path: str, size: Tuple[int, int] = (600, 600) -) -> bool: +def image_util_generate_thumbnail( + image_path: str, thumbnail_path: str, size: Tuple[int, int] = None +) -> bool:Then (outside this hunk), refine the implementation:
# imports from PIL import Image, ImageOps # inside function, before opening image size = size or getattr(__import__("app.config.settings").config.settings, "THUMBNAIL_DEFAULT_SIZE", (600, 600)) os.makedirs(os.path.dirname(thumbnail_path), exist_ok=True) with Image.open(image_path) as img: # fix orientation from EXIF img = ImageOps.exif_transpose(img) # high-quality downscale img.thumbnail(size, resample=getattr(Image, "Resampling", Image).LANCZOS) if img.mode not in ("RGB", "L"): img = img.convert("RGB") # better compression for thumbnails img.save(thumbnail_path, "JPEG", quality=80, optimize=True, progressive=True)Optional: cap max input pixels via a setting to avoid decompression-bomb images (guardrail for user-supplied files).
frontend/src/pages/Memories/Memories.tsx (1)
1-6: Memories page is inert—redirect or hide until readyReturning an empty fragment degrades UX and masks navigation issues. Prefer redirecting or removing from nav/tests until implemented.
Apply:
-const Memories = () => { - return <></>; -}; +import { Navigate } from 'react-router-dom'; +import { ROUTES } from '@/constants/routes'; +const Memories = () => <Navigate to={`/${ROUTES.HOME}`} replace />;frontend/src/pages/Album/Album.tsx (1)
1-6: Album page returns nothing—avoid shipping no-op pagesEither deprecate the route or redirect to Home so users don’t see a blank page.
Apply:
-function Album() { - return <></>; -} +import { Navigate } from 'react-router-dom'; +import { ROUTES } from '@/constants/routes'; +function Album() { + return <Navigate to={`/${ROUTES.HOME}`} replace />; +}frontend/src/pages/__tests__/allPages.test.tsx (1)
10-10: Fix BrowserRouter import sourceBrowserRouter comes from react-router-dom, not react-router. Tests will fail at compile/runtime otherwise.
Apply:
-import { BrowserRouter } from 'react-router'; +import { BrowserRouter } from 'react-router-dom';backend/app/utils/face_clusters.py (1)
296-308: Guard against zero-norm embeddings to avoid NaNs/inf.Normalization can divide by zero if an embedding is all zeros.
- face_norm = face_embedding / np.linalg.norm(face_embedding) + eps = 1e-8 + denom_face = max(np.linalg.norm(face_embedding), eps) + face_norm = face_embedding / denom_face @@ - cluster_norms = cluster_means / np.linalg.norm(cluster_means, axis=1, keepdims=True) + cluster_denoms = np.linalg.norm(cluster_means, axis=1, keepdims=True) + cluster_denoms = np.maximum(cluster_denoms, eps) + cluster_norms = cluster_means / cluster_denomsbackend/app/routes/folders.py (1)
149-156: Use 403 Forbidden for filesystem permission errors (not 401).
401 implies authentication. This case is authorization/permission.Apply this diff:
- raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, detail=ErrorResponse( success=False, error="Permission denied", message="The app does not have read permission for the specified folder", ).model_dump(), )
♻️ Duplicate comments (6)
frontend/src-tauri/tauri.conf.json (1)
63-64: Fix CSP: add connect-src (ipc/localhost) and data/blob for images; provide a looser devCsp.As-is, IPC/localhost requests will be blocked (no connect-src), and base64/blob thumbnails won’t render (img-src lacks data:/blob:). Also add basic hardening (object-src/base-uri/frame-ancestors).
Apply:
"csp": "default-src 'self' ipc: http://ipc.localhost; img-src 'self' asset: http://asset.localhost" + "csp": "default-src 'self'; object-src 'none'; base-uri 'self'; frame-ancestors 'none'; img-src 'self' asset: data: blob:; font-src 'self' asset:; media-src 'self' asset: data: blob:; connect-src 'self' ipc: http://ipc.localhost http://asset.localhost" + , + "devCsp": "default-src 'self'; img-src 'self' asset: data: blob:; font-src 'self' asset:; media-src 'self' asset: data: blob:; connect-src 'self' ipc: http://localhost:* http://127.0.0.1:* ws://localhost:*; style-src 'self' 'unsafe-inline'"backend/app/database/images.py (1)
134-147: Cast folder_id to str and dedupe tags (aligns FE contract; avoids duplicates).folder_id is emitted as an int while FolderId alias is str and FE schemas use strings; also tags can duplicate due to joins.
Apply:
- images_dict[image_id] = { + images_dict[image_id] = { "id": image_id, "path": path, - "folder_id": folder_id, + "folder_id": str(folder_id), "thumbnailPath": thumbnail_path, "metadata": metadata, "isTagged": bool(is_tagged), "tags": [], }- if tag_name: - images_dict[image_id]["tags"].append(tag_name) + if tag_name and tag_name not in images_dict[image_id]["tags"]: + images_dict[image_id]["tags"].append(tag_name)backend/app/utils/microservice.py (1)
56-66: Make startup idempotent: skip if port 8001 is already in use.Prevents duplicate processes and flakiness on repeated app starts.
# Install dependencies if requirements.txt exists requirements_file = sync_service_path / "requirements.txt" if requirements_file.exists(): logger.info("Installing dependencies...") if not _install_requirements(python_executable, requirements_file): logger.warning("Failed to install requirements, but continuing...") - # Start the FastAPI service + # Start the FastAPI service (idempotent if already running) + if _is_port_open("127.0.0.1", 8001): + logger.info("Sync microservice already running on port 8001") + return True logger.info("Starting sync microservice on port 8001...") return _start_fastapi_service(python_executable, sync_service_path)Add helper (outside this hunk):
import socket def _is_port_open(host: str, port: int) -> bool: with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: s.settimeout(0.5) return s.connect_ex((host, port)) == 0backend/main.py (1)
45-45: Don’t block startup; gate microservice bootstrap and offload to a thread.Prevents event-loop blocking and enables opt-in behavior.
- microservice_util_start_sync_service() + if os.getenv("START_SYNC_MICROSERVICE", "false").lower() == "true": + import asyncio + await asyncio.to_thread(microservice_util_start_sync_service)Also consider returning a process handle from the util and terminating it on shutdown.
backend/app/routes/folders.py (2)
70-72: Don't ignore restart outcome; handle failure.
Same concern as earlier review; check return and log/handle.Apply this diff:
- # Restart sync microservice watcher after processing images - API_util_restart_sync_microservice_watcher() + # Restart sync microservice watcher after processing images + try: + if not API_util_restart_sync_microservice_watcher(): + print("Warning: Failed to restart sync microservice watcher") + except Exception as err: + print(f"Warning: Restart sync microservice watcher threw: {err}")
117-119: Same as above: handle restart failures during sync.
Mirror the handling added to post-add.Apply this diff:
- # Restart sync microservice watcher after processing images - API_util_restart_sync_microservice_watcher() + # Restart sync microservice watcher after processing images + try: + if not API_util_restart_sync_microservice_watcher(): + print("Warning: Failed to restart sync microservice watcher") + except Exception as err: + print(f"Warning: Restart sync microservice watcher threw: {err}")
🧹 Nitpick comments (19)
frontend/src/components/OnboardingSteps/AppFeatures.tsx (2)
4-8: Broaden icon type; hide decorative emoji from AT.Allow React nodes (e.g., SVG icon components) and hide the emoji from assistive tech to avoid duplicate announcements.
interface Feature { title: string; description: string; - icon: string; + icon: React.ReactNode; }Also update the icon render:
- <div className="text-5xl">{currentFeature.icon}</div> + <div className="text-5xl" aria-hidden="true">{currentFeature.icon}</div>
28-28: Prefer function component over React.FC.Avoids implicit children and aligns with current TS/React guidance.
-export const AppFeatures: React.FC = () => { +export function AppFeatures() {frontend/src/pages/PersonImages/PersonImages.tsx (4)
35-47: Show loader during refetch as wellCurrently loader only shows for initial load. Include isFetching and update deps.
- useEffect(() => { - if (isLoading) { + useEffect(() => { + if (isLoading || isFetching) { dispatch(showLoader('Loading images')); } else if (isError) { dispatch(hideLoader()); } else if (isSuccess) { - const res: any = data?.data; - const images = (res?.images || []) as Image[]; - dispatch(setImages(images)); - setClusterName(res?.cluster_name || 'random_name'); + // typed below in a separate change + const res: ClusterImagesPayload = (data?.data || {}) as ClusterImagesPayload; + const fetchedImages = res.images ?? []; + dispatch(setImages(fetchedImages)); + setClusterName(res.cluster_name ?? 'random_name'); dispatch(hideLoader()); } - }, [data, isSuccess, isError, isLoading, dispatch]); + }, [data, isSuccess, isError, isLoading, isFetching, dispatch]);
41-45: Strongly type response and avoid shadowingimagesImproves type safety and readability.
- const res: any = data?.data; - const images = (res?.images || []) as Image[]; - dispatch(setImages(images)); - setClusterName(res?.cluster_name || 'random_name'); + type ClusterImagesPayload = { images: Image[]; cluster_name?: string }; + const res = (data?.data || {}) as ClusterImagesPayload; + const fetchedImages = res.images ?? []; + dispatch(setImages(fetchedImages)); + setClusterName(res.cluster_name ?? 'random_name');
77-82: Small UX polish: Enter-to-save and disable Save on emptyPrevents accidental empty names and supports keyboard flow.
<Input value={clusterName} onChange={handleNameChange} + onKeyDown={(e) => e.key === 'Enter' && handleSaveName()} className="max-w-xs" placeholder="Enter person name" /> <Button variant="outline" size="icon" onClick={handleSaveName} + disabled={!clusterName.trim()} className="h-10 w-10" >Also applies to: 83-91
105-113: Consider list virtualization for large clustersRendering all cards can be heavy. Use react-window/react-virtualized or an infinite grid to keep initial render and memory low.
frontend/src/pages/__tests__/allPages.test.tsx (1)
27-33: Test matrix includes inert pages; tighten assertions or drop until implementedAlbum/Memories currently render nothing, so these tests only assert “no crash” and can’t detect regressions. Either remove them from the pages array for now or add assertions for meaningful UI on implemented pages (e.g., Home “Image Gallery”, AI “AI Tagging”).
Apply (temporary removal):
{ path: ROUTES.AI, Component: AITagging }, - { path: ROUTES.ALBUMS, Component: Album }, - { path: ROUTES.MEMORIES, Component: Memories }, + // TODO(GSoC-Revamp): Re-enable when pages are implemented + // { path: ROUTES.ALBUMS, Component: Album }, + // { path: ROUTES.MEMORIES, Component: Memories },Optionally, add basic assertions for implemented pages to catch blank renders:
// Example: const { getByText } = render(...<Home />...); expect(getByText('Image Gallery')).toBeInTheDocument();frontend/src/pages/Home/Home.tsx (3)
24-26: Surface API errors to users (not just hide loader).Consider showing a toast/dialog or setting an error slice with errorMessage from the hook to improve UX.
33-35: Remove no-op onClose handler for MediaView.MediaView already closes via Redux; the prop here does nothing.
- const handleCloseMediaView = () => { - // MediaView will handle closing via Redux - }; + // MediaView handles closing via Redux ... - {isImageViewOpen && <MediaView onClose={handleCloseMediaView} />} + {isImageViewOpen && <MediaView />}Also applies to: 54-54
41-51: Add an empty state for zero images.Improves clarity when the gallery is empty.
- <div className="grid grid-cols-1 gap-4 sm:grid-cols-2 md:grid-cols-3 lg:grid-cols-4 xl:grid-cols-5"> - {images.map((image, index) => ( - <ImageCard - key={image.id} - image={image} - imageIndex={index} - className="w-full" - /> - ))} - </div> + {images.length === 0 ? ( + <div className="text-sm text-muted-foreground">No images found.</div> + ) : ( + <div className="grid grid-cols-1 gap-4 sm:grid-cols-2 md:grid-cols-3 lg:grid-cols-4 xl:grid-cols-5"> + {images.map((image, index) => ( + <ImageCard + key={image.id} + image={image} + imageIndex={index} + className="w-full" + /> + ))} + </div> + )}backend/app/database/images.py (2)
156-158: Avoid double sorting.You already ORDER BY i.path in SQL; either keep that or the Python sort, not both. Dropping the Python sort saves O(n log n).
- # Sort by path - images.sort(key=lambda x: x["path"]) -
161-165: Use logging instead of print for DB errors.Switch to logger to preserve stack traces and integrate with app logging.
- except Exception as e: - print(f"Error getting all images: {e}") + except Exception: + logger.exception("Error getting all images") return []Add once at top of module (outside this hunk):
import logging logger = logging.getLogger(__name__)backend/app/utils/face_clusters.py (2)
163-177: Replace debug prints with logger calls.Keep logs consistent and configurable.
- print(f"Total faces to cluster: {len(face_ids)}") + logger.info("Total faces to cluster: %d", len(face_ids)) @@ - print(f"DBSCAN found {len(set(cluster_labels)) - 1} clusters") + logger.info("DBSCAN found %d clusters", len(set(cluster_labels)) - 1)Add once at top (outside this hunk):
import logging logger = logging.getLogger(__name__)
311-344: Standardize error reporting: use logger.exception instead of print across helpers.Multiple helpers print on exceptions. Switch to
logger.exception(...)for stack traces and centralized logging.Also applies to: 346-394, 461-492, 494-511, 513-550
backend/app/routes/face_clusters.py (2)
110-124: Large payload risk: face_image_base64 in list response.
Embedding base64 previews in collection responses can bloat payloads. Consider omitting it here or adding a query flag (e.g., include_face_image=false by default).
137-196: Consider pagination for cluster images.
Add limit/offset (and total) to avoid returning unbounded lists on large clusters.backend/app/routes/folders.py (3)
192-195: Good: AddFolderResponse now uses data envelope. Also fix type hint drift.
root_folder_id is a UUID string; update post_folder_add_sequence signature to str.Additional changes outside this hunk:
# Change signature (elsewhere in this file) def post_folder_add_sequence(folder_path: str, folder_id: str): ...
390-397: Good: Sync response envelope. Also align types for post_sync helper.
request.folder_id is str; adjust post_sync_folder_sequence signature accordingly.Additional changes outside this hunk:
# Change signature (elsewhere in this file) from typing import List, Tuple def post_sync_folder_sequence( folder_path: str, folder_id: str, added_folders: List[Tuple[str, str]] ): ...
430-471: GET /all-folders implemented cleanly. Consider pagination.
Add limit/offset and perhaps ordering params to prevent large responses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (39)
backend/app/database/face_clusters.py(7 hunks)backend/app/database/folders.py(1 hunks)backend/app/database/images.py(1 hunks)backend/app/models/FaceDetector.py(2 hunks)backend/app/routes/face_clusters.py(4 hunks)backend/app/routes/folders.py(11 hunks)backend/app/schemas/folders.py(1 hunks)backend/app/utils/API.py(1 hunks)backend/app/utils/face_clusters.py(4 hunks)backend/app/utils/images.py(2 hunks)backend/app/utils/microservice.py(1 hunks)backend/main.py(3 hunks)frontend/src-tauri/tauri.conf.json(1 hunks)frontend/src/App.tsx(1 hunks)frontend/src/components/Dialog/InfoDialog.tsx(2 hunks)frontend/src/components/FaceCollections.tsx(1 hunks)frontend/src/components/FolderPicker/DeleteSelectedImagePage.tsx(0 hunks)frontend/src/components/Navigation/Sidebar/AppSidebar.tsx(1 hunks)frontend/src/components/OnboardingSteps/AppFeatures.tsx(1 hunks)frontend/src/components/OnboardingSteps/OnboardingStep.tsx(1 hunks)frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx(1 hunks)frontend/src/components/SetupScreen.tsx(0 hunks)frontend/src/components/ui/ProgressiveLoader.tsx(0 hunks)frontend/src/components/ui/aspect-ratio.tsx(1 hunks)frontend/src/components/ui/avatar.tsx(1 hunks)frontend/src/components/ui/sheet.tsx(1 hunks)frontend/src/components/ui/skeleton.tsx(1 hunks)frontend/src/components/ui/switch.tsx(1 hunks)frontend/src/features/infoDialogSlice.ts(1 hunks)frontend/src/hooks/use-mobile.ts(1 hunks)frontend/src/hooks/useImages.ts(0 hunks)frontend/src/pages/Album/Album.tsx(1 hunks)frontend/src/pages/Home/Home.tsx(1 hunks)frontend/src/pages/Memories/Memories.tsx(1 hunks)frontend/src/pages/PersonImages/PersonImages.tsx(1 hunks)frontend/src/pages/SecureFolderPage/SecureFolder.tsx(1 hunks)frontend/src/pages/VideosPage/Videos.tsx(1 hunks)frontend/src/pages/__tests__/allPages.test.tsx(2 hunks)frontend/src/utils/Media.ts(0 hunks)
💤 Files with no reviewable changes (5)
- frontend/src/utils/Media.ts
- frontend/src/components/SetupScreen.tsx
- frontend/src/hooks/useImages.ts
- frontend/src/components/FolderPicker/DeleteSelectedImagePage.tsx
- frontend/src/components/ui/ProgressiveLoader.tsx
✅ Files skipped from review due to trivial changes (3)
- frontend/src/App.tsx
- frontend/src/components/Dialog/InfoDialog.tsx
- frontend/src/features/infoDialogSlice.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- frontend/src/components/ui/skeleton.tsx
- frontend/src/components/ui/switch.tsx
- frontend/src/components/FaceCollections.tsx
- backend/app/utils/API.py
- frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx
- frontend/src/components/ui/avatar.tsx
- frontend/src/components/ui/aspect-ratio.tsx
- backend/app/database/face_clusters.py
- frontend/src/hooks/use-mobile.ts
- frontend/src/components/Navigation/Sidebar/AppSidebar.tsx
- backend/app/database/folders.py
- frontend/src/components/ui/sheet.tsx
- backend/app/models/FaceDetector.py
- frontend/src/components/OnboardingSteps/OnboardingStep.tsx
🧰 Additional context used
🧬 Code graph analysis (10)
frontend/src/components/OnboardingSteps/AppFeatures.tsx (1)
frontend/src/components/ui/card.tsx (2)
Card(85-85)CardContent(91-91)
frontend/src/pages/Home/Home.tsx (8)
frontend/src/features/imageSelectors.ts (2)
selectIsImageViewOpen(22-25)selectImages(5-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/Media/ImageCard.tsx (1)
ImageCard(20-111)frontend/src/components/Media/MediaView.tsx (1)
MediaView(31-192)
frontend/src/pages/VideosPage/Videos.tsx (1)
frontend/src/components/ui/LoadingScreen/LoadingScreen.tsx (1)
LoadingScreen(15-64)
frontend/src/pages/PersonImages/PersonImages.tsx (9)
frontend/src/features/imageSelectors.ts (2)
selectIsImageViewOpen(22-25)selectImages(5-5)frontend/src/hooks/useQueryExtension.ts (2)
usePictoQuery(80-108)usePictoMutation(26-78)frontend/src/api/api-functions/face_clusters.ts (2)
fetchClusterImages(31-38)renameCluster(21-29)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/constants/routes.ts (1)
ROUTES(1-11)frontend/src/components/Media/ImageCard.tsx (1)
ImageCard(20-111)frontend/src/components/Media/MediaView.tsx (1)
MediaView(31-192)
backend/app/schemas/folders.py (2)
frontend/src/types/Folder.ts (2)
FolderDetails(1-8)GetAllFoldersData(10-13)backend/app/schemas/face_clusters.py (1)
ErrorResponse(23-26)
backend/app/routes/folders.py (3)
backend/app/database/folders.py (1)
db_get_all_folder_details(385-406)backend/app/schemas/folders.py (15)
AddFolderRequest(6-9)AddFolderResponse(72-76)AddFolderData(40-42)UpdateAITaggingRequest(12-13)UpdateAITaggingResponse(79-83)UpdateAITaggingData(45-47)DeleteFoldersRequest(16-17)DeleteFoldersResponse(86-90)DeleteFoldersData(50-52)SyncFolderRequest(20-22)SyncFolderResponse(93-97)SyncFolderData(55-61)GetAllFoldersResponse(65-69)GetAllFoldersData(35-37)FolderDetails(26-32)backend/app/utils/API.py (1)
API_util_restart_sync_microservice_watcher(8-33)
backend/app/utils/face_clusters.py (2)
backend/app/database/face_clusters.py (2)
db_delete_all_clusters(179-195)db_insert_clusters_batch(38-77)backend/app/database/faces.py (1)
db_update_face_cluster_ids_batch(198-238)
backend/main.py (1)
backend/app/utils/microservice.py (1)
microservice_util_start_sync_service(13-69)
frontend/src/pages/__tests__/allPages.test.tsx (3)
frontend/src/constants/routes.ts (1)
ROUTES(1-11)frontend/src/pages/Home/Home.tsx (1)
Home(12-57)frontend/src/pages/AITagging/AITagging.tsx (1)
AITagging(16-71)
backend/app/routes/face_clusters.py (2)
backend/app/database/face_clusters.py (2)
db_get_images_by_cluster_id(242-309)db_get_cluster_by_id(80-105)backend/app/schemas/face_clusters.py (10)
RenameClusterRequest(6-7)RenameClusterResponse(16-20)RenameClusterData(11-13)ErrorResponse(23-26)GetClustersResponse(40-44)GetClustersData(36-37)ClusterMetadata(29-33)GetClusterImagesResponse(68-74)GetClusterImagesData(59-65)ImageInCluster(47-56)
⏰ 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: Tauri Tests
🔇 Additional comments (17)
backend/app/utils/images.py (1)
149-151: Store only relative/public paths in DB/API and reserve absolute FS paths for I/O
- In backend/app/utils/images.py (lines 149–151), replace
withthumbnail_path = os.path.abspath(os.path.join(THUMBNAIL_IMAGES_PATH, thumbnail_name))then persistthumbnail_fs_path = os.path.abspath(os.path.join(THUMBNAIL_IMAGES_PATH, thumbnail_name)) thumbnail_public_path = os.path.join(Path(THUMBNAIL_IMAGES_PATH).name, thumbnail_name)thumbnail_public_pathin yourimage_recordsand usethumbnail_fs_pathfor filesystem operations.- Update the removal helper (
image_util_remove_obsolete_images) to resolve a relativethumbnailPathback to its FS path:thumb_fs_path = ( thumbnail_path if os.path.isabs(thumbnail_path) else os.path.join(THUMBNAIL_IMAGES_PATH, thumbnail_path) ) if thumbnail_path and os.path.exists(thumb_fs_path): os.remove(thumb_fs_path)- Propagate this change through the stack:
• Database layer (backend/app/database/images.py, face_clusters.py) continues storing thethumbnailPathcolumn as TEXT (now holding the relative/public path).
• Pydantic schemas (backend/app/schemas/images.py, face_clusters.py) accept the relative/publicthumbnailPath.
• Route handlers (backend/app/routes/images.py, face_clusters.py) return the relative/public path (and may prepend a URL prefix if needed).frontend/src/components/OnboardingSteps/AppFeatures.tsx (3)
37-38: LGTM on interval cleanup.Cleanup is correctly registered; prevents leaked timers on unmount.
10-26: Introduce an i18n solution and externalize these strings.
No i18n dependencies or locale directories were detected—confirm whether localization is required and, if so, wire thetitle,description(and any other user-facing text) through your chosen i18n framework rather than leaving them hard-coded inAppFeatures.tsx.
2-2: Alias@mapping verified. tsconfig.json defines"paths": { "@/*": ["./src/*"] }and vite.config.ts setsalias: { '@': path.resolve(__dirname, './src') }, so imports using@/…will resolve correctly.frontend/src/pages/SecureFolderPage/SecureFolder.tsx (1)
1-5: Remove orphaned SecureFolder page
TheSecureFoldercomponent isn’t referenced in any<Route>(seeAppRoutes.tsx), so there’s no blank route to hit. Deletefrontend/src/pages/SecureFolderPage(or add it to your routes) as appropriate.Likely an incorrect or invalid review comment.
frontend/src/pages/__tests__/allPages.test.tsx (1)
2-5: Good switch to named exports for AITagging and HomeMatches the updated components’ public APIs and avoids default-import drift.
frontend/src/pages/Home/Home.tsx (2)
12-12: Export/import consistency confirmed Named exportHomematches all route and test imports.
10-10: No change needed for import path
ThefetchAllImagesfunction is re-exported in the barrel filefrontend/src/api/api-functions/index.ts(export * from './images';), so importing from@/api/api-functionsis valid.backend/app/utils/face_clusters.py (1)
95-117: Ignore FK reordering suggestion; ON DELETE SET NULL makes delete-all-safe. Thefaces.cluster_idforeign key is defined withON DELETE SET NULL, so deleting all clusters first won’t violate constraints, and the existing delete–insert–update sequence works as intended.Likely an incorrect or invalid review comment.
backend/app/schemas/folders.py (1)
65-70: Envelope response looks good and matches FE types.Consistent success/message/error/data pattern; aligns with new FE wrappers.
backend/main.py (2)
116-116: Images router mount LGTM.Public /images endpoint wiring is clear.
109-111: Health endpoint tags LGTM.Simple and discoverable in OpenAPI.
backend/app/routes/face_clusters.py (2)
6-19: Imports for new schemas and DB API look correct.
71-74: Good: wrap rename response in data envelope.
Aligns with schemas.RenameClusterData.backend/app/routes/folders.py (3)
238-240: Good: UpdateAITaggingResponse uses data payload.
279-281: Good: Disable AI tagging response envelope.
320-322: Good: DeleteFoldersResponse envelope with ids.
| images = [ | ||
| ImageInCluster( | ||
| id=img["image_id"], | ||
| path=img["image_path"], | ||
| thumbnailPath=img["thumbnail_path"], | ||
| metadata=img["metadata"], | ||
| face_id=img["face_id"], | ||
| confidence=img["confidence"], | ||
| bbox=img["bbox"], | ||
| ) | ||
| for img in images_data |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cast image_id to str to match schema.
ImageInCluster.id is str; DB returns int. Cast to avoid reliance on implicit coercion.
Apply this diff:
- ImageInCluster(
- id=img["image_id"],
+ ImageInCluster(
+ id=str(img["image_id"]),
path=img["image_path"],
thumbnailPath=img["thumbnail_path"],
metadata=img["metadata"],
face_id=img["face_id"],
confidence=img["confidence"],
bbox=img["bbox"],
)📝 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.
| images = [ | |
| ImageInCluster( | |
| id=img["image_id"], | |
| path=img["image_path"], | |
| thumbnailPath=img["thumbnail_path"], | |
| metadata=img["metadata"], | |
| face_id=img["face_id"], | |
| confidence=img["confidence"], | |
| bbox=img["bbox"], | |
| ) | |
| for img in images_data | |
| images = [ | |
| ImageInCluster( | |
| id=str(img["image_id"]), | |
| path=img["image_path"], | |
| thumbnailPath=img["thumbnail_path"], | |
| metadata=img["metadata"], | |
| face_id=img["face_id"], | |
| confidence=img["confidence"], | |
| bbox=img["bbox"], | |
| ) | |
| for img in images_data | |
| ] |
🤖 Prompt for AI Agents
In backend/app/routes/face_clusters.py around lines 161 to 171, the
ImageInCluster.id field is defined as a str but the DB returns an int; update
the list comprehension to cast image_id to a string when constructing
ImageInCluster (e.g., id=str(img["image_id"])), ensuring any non-string DB
values are converted to match the schema and avoid implicit coercion.
| cluster_id=cluster_id, | ||
| cluster_name=cluster["cluster_name"], | ||
| images=images, | ||
| total_images=len(images), |
There was a problem hiding this comment.
Bug: cluster_name access uses dict indexing on an object.
db_get_cluster_by_id returns a ClusterData object, not a dict.
Apply this diff:
- cluster_id=cluster_id,
- cluster_name=cluster["cluster_name"],
+ cluster_id=cluster_id,
+ cluster_name=cluster.cluster_name,📝 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.
| cluster_id=cluster_id, | |
| cluster_name=cluster["cluster_name"], | |
| images=images, | |
| total_images=len(images), | |
| cluster_id=cluster_id, | |
| cluster_name=cluster.cluster_name, | |
| images=images, | |
| total_images=len(images), |
🤖 Prompt for AI Agents
In backend/app/routes/face_clusters.py around lines 178 to 181, the code treats
the returned ClusterData as a dict by using cluster["cluster_name"]; change this
to use attribute access (cluster.cluster_name) and any similar dict-style
accesses on the ClusterData object to be dot-attribute accesses so the code
reads cluster.cluster_name (and adjust other fields if present) before
constructing the response.
| def microservice_util_start_sync_service( | ||
| sync_service_path: Optional[str] = None, | ||
| ) -> bool: | ||
| """ | ||
| Start the sync microservice with automatic virtual environment management. | ||
|
|
||
| Args: | ||
| sync_service_path: Path to the sync microservice directory. | ||
| If None, defaults to 'sync-microservice' relative to project root. | ||
|
|
||
| Returns: | ||
| bool: True if service started successfully, False otherwise. | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return a handle (PID or Popen) instead of bool to enable managed shutdown.
Returning only bool prevents proper lifecycle management from main.py. Recommend returning subprocess.Popen (or PID) and storing it to terminate on app shutdown.
🤖 Prompt for AI Agents
In backend/app/utils/microservice.py around lines 13 to 25, the function
microservice_util_start_sync_service currently returns a bool which prevents
consumers from managing the subprocess lifecycle; change the signature and
implementation to return a subprocess.Popen (or PID) instead of bool, create the
process with subprocess.Popen (ensuring stdout/stderr and cwd/env are set
appropriately and errors are handled), propagate any startup failures by
returning None or raising, and update callers to store the returned Popen and
terminate/communicate it on application shutdown for proper managed shutdown.
| # Command to start FastAPI dev server | ||
| print(python_executable) | ||
| cmd = [str(python_executable), "-m", "fastapi", "dev", "--port", "8001"] | ||
|
|
||
| # Start the process (non-blocking) | ||
| process = subprocess.Popen( | ||
| cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True | ||
| ) | ||
|
|
||
| # Restore original working directory | ||
| os.chdir(original_cwd) | ||
|
|
||
| logger.info(f"Sync microservice started with PID: {process.pid}") | ||
| logger.info("Service should be available at http://localhost:8001") | ||
|
|
||
| return True | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix server invocation and pipe handling; avoid deadlock and missing app module.
- fastapi dev without an app path will likely fail; prefer uvicorn main:app.
- Piping stdout/stderr without consuming can block; redirect safely.
- Remove stray print.
- Optionally wait for readiness.
- # Command to start FastAPI dev server
- print(python_executable)
- cmd = [str(python_executable), "-m", "fastapi", "dev", "--port", "8001"]
+ # Command to start FastAPI with uvicorn (expects main.py with `app`)
+ cmd = [
+ str(python_executable),
+ "-m",
+ "uvicorn",
+ "main:app",
+ "--host", "127.0.0.1",
+ "--port", "8001",
+ "--workers", "1",
+ "--reload",
+ ]
- # Start the process (non-blocking)
- process = subprocess.Popen(
- cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True
- )
+ # Start the process (non-blocking). Do not pipe to avoid buffer deadlocks.
+ process = subprocess.Popen(
+ cmd,
+ stdout=subprocess.DEVNULL,
+ stderr=subprocess.DEVNULL,
+ start_new_session=True,
+ )
# Restore original working directory
os.chdir(original_cwd)
logger.info(f"Sync microservice started with PID: {process.pid}")
- logger.info("Service should be available at http://localhost:8001")
+ # Best-effort readiness wait
+ for _ in range(20): # ~10s
+ if _is_port_open("127.0.0.1", 8001):
+ break
+ time.sleep(0.5)
+ logger.info("Service should be available at http://localhost:8001")
return TrueAlso add at file top (outside this hunk): import time.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/app/utils/microservice.py around lines 159 to 175, fix the server
invocation and pipe handling: remove the stray print, change the command to run
a specific ASGI app (for example: [str(python_executable), "-m", "uvicorn",
"main:app", "--port", "8001"]) instead of "fastapi dev", redirect stdout and
stderr to subprocess.DEVNULL or to a log file (avoid subprocess.PIPE without a
consumer to prevent deadlock), restore cwd, log the PID and URL as before, and
optionally add a simple readiness wait (import time at the top and poll/sleep
briefly or attempt a local socket/connect retry) before returning True.
| useEffect(() => { | ||
| const interval = setInterval(() => { | ||
| setFeatureIndex((prev) => (prev + 1) % features.length); | ||
| }, 3000); | ||
|
|
||
| return () => clearInterval(interval); | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Auto-advance should respect prefers-reduced-motion and allow pausing.
Add reduced-motion detection and a pause flag; gate the interval on those states.
useEffect(() => {
- const interval = setInterval(() => {
- setFeatureIndex((prev) => (prev + 1) % features.length);
- }, 3000);
-
- return () => clearInterval(interval);
- }, []);
+ if (isPaused || reducedMotion) return;
+ const interval = setInterval(() => {
+ setFeatureIndex((prev) => (prev + 1) % features.length);
+ }, 3000);
+ return () => clearInterval(interval);
+ }, [isPaused, reducedMotion]);
+
+ useEffect(() => {
+ const mql = window.matchMedia?.('(prefers-reduced-motion: reduce)');
+ if (!mql) return;
+ const onChange = (e: MediaQueryListEvent) => setReducedMotion(e.matches);
+ setReducedMotion(mql.matches);
+ mql.addEventListener('change', onChange);
+ return () => mql.removeEventListener('change', onChange);
+ }, []);Add state declarations (place just after featureIndex):
- const currentFeature = features[featureIndex];
+ const [isPaused, setIsPaused] = useState(false);
+ const [reducedMotion, setReducedMotion] = useState(false);
+ const currentFeature = features[featureIndex];📝 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.
| useEffect(() => { | |
| const interval = setInterval(() => { | |
| setFeatureIndex((prev) => (prev + 1) % features.length); | |
| }, 3000); | |
| return () => clearInterval(interval); | |
| }, []); | |
| // after your existing featureIndex state… | |
| const [featureIndex, setFeatureIndex] = useState(0); | |
| const [isPaused, setIsPaused] = useState(false); | |
| const [reducedMotion, setReducedMotion] = useState(false); | |
| const currentFeature = features[featureIndex]; | |
| - useEffect(() => { | |
| - const interval = setInterval(() => { | |
| - setFeatureIndex((prev) => (prev + 1) % features.length); | |
| - }, 3000); | |
| - | |
| - return () => clearInterval(interval); | |
| useEffect(() => { | |
| if (isPaused || reducedMotion) return; | |
| const interval = setInterval(() => { | |
| setFeatureIndex((prev) => (prev + 1) % features.length); | |
| }, 3000); | |
| return () => clearInterval(interval); | |
| }, [isPaused, reducedMotion]); | |
| // listen for prefers-reduced-motion changes | |
| useEffect(() => { | |
| const mql = window.matchMedia?.('(prefers-reduced-motion: reduce)'); | |
| if (!mql) return; | |
| const onChange = (e: MediaQueryListEvent) => setReducedMotion(e.matches); | |
| setReducedMotion(mql.matches); | |
| mql.addEventListener('change', onChange); | |
| return () => mql.removeEventListener('change', onChange); | |
| }, []); |
🤖 Prompt for AI Agents
In frontend/src/components/OnboardingSteps/AppFeatures.tsx around lines 32 to
38, add two state declarations just after featureIndex: a paused state (const
[paused, setPaused] = useState(false)) and a prefersReducedMotion state (const
[prefersReducedMotion, setPrefersReducedMotion] = useState(false)) that you
initialize via window.matchMedia('(prefers-reduced-motion: reduce)').matches and
update with a listener in a useEffect (remember to cleanup the listener). Then
update the existing interval useEffect to only create the interval when paused
is false and prefersReducedMotion is false (otherwise do not start it), and
ensure you clear the interval on cleanup; expose setPaused to the component
UI/interaction so users can toggle pausing.
| const { data, isLoading, isSuccess, isError } = usePictoQuery({ | ||
| queryKey: ['person-images', clusterId], | ||
| queryFn: async () => fetchClusterImages({ clusterId: clusterId || '' }), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid empty clusterId requests; gate the query and use non-null assertion in queryFn
Prevents a call with '' and enables refetch/use of isFetching/refetch.
- const { data, isLoading, isSuccess, isError } = usePictoQuery({
- queryKey: ['person-images', clusterId],
- queryFn: async () => fetchClusterImages({ clusterId: clusterId || '' }),
- });
+ const { data, isLoading, isFetching, isSuccess, isError, refetch } =
+ usePictoQuery({
+ queryKey: ['person-images', clusterId],
+ enabled: !!clusterId,
+ queryFn: () => fetchClusterImages({ clusterId: clusterId! }),
+ });📝 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 { data, isLoading, isSuccess, isError } = usePictoQuery({ | |
| queryKey: ['person-images', clusterId], | |
| queryFn: async () => fetchClusterImages({ clusterId: clusterId || '' }), | |
| }); | |
| const { data, isLoading, isFetching, isSuccess, isError, refetch } = | |
| usePictoQuery({ | |
| queryKey: ['person-images', clusterId], | |
| enabled: !!clusterId, | |
| queryFn: () => fetchClusterImages({ clusterId: clusterId! }), | |
| }); |
🤖 Prompt for AI Agents
In frontend/src/pages/PersonImages/PersonImages.tsx around lines 25 to 28, the
query currently runs with an empty string when clusterId is falsy which triggers
unnecessary requests and prevents proper refetching/isFetching behavior; update
the usePictoQuery call to gate execution by adding an enabled flag (e.g.,
enabled: Boolean(clusterId) or !!clusterId) so the query only runs when
clusterId is present, and inside queryFn use a non-null assertion for clusterId
(clusterId!) or otherwise cast it to a non-empty string to avoid passing '' to
fetchClusterImages; keep the queryKey keyed by clusterId so refetch and
isFetching work correctly.
| const { mutate: renameClusterMutate } = usePictoMutation({ | ||
| mutationFn: async (newName: string) => | ||
| renameCluster({ clusterId: clusterId || '', newName }), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make rename mutation update UI and invalidate related queries
Also prevents stale views if other parts depend on this key.
- const { mutate: renameClusterMutate } = usePictoMutation({
- mutationFn: async (newName: string) =>
- renameCluster({ clusterId: clusterId || '', newName }),
- });
+ const { mutate: renameClusterMutate } = usePictoMutation({
+ mutationFn: (newName: string) =>
+ renameCluster({ clusterId: clusterId!, newName }),
+ onSuccess: (_data, newName) => {
+ // Trust server if it returns canonical name via _data?.data?.cluster_name
+ setClusterName(newName);
+ },
+ autoInvalidateTags: ['person-images', clusterId ?? ''],
+ });📝 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 { mutate: renameClusterMutate } = usePictoMutation({ | |
| mutationFn: async (newName: string) => | |
| renameCluster({ clusterId: clusterId || '', newName }), | |
| }); | |
| const { mutate: renameClusterMutate } = usePictoMutation({ | |
| - mutationFn: async (newName: string) => | |
| - renameCluster({ clusterId: clusterId || '', newName }), | |
| mutationFn: (newName: string) => | |
| renameCluster({ clusterId: clusterId!, newName }), | |
| onSuccess: (_data, newName) => { | |
| // Trust server if it returns canonical name via _data?.data?.cluster_name | |
| setClusterName(newName); | |
| }, | |
| autoInvalidateTags: ['person-images', clusterId ?? ''], | |
| }); |
🤖 Prompt for AI Agents
In frontend/src/pages/PersonImages/PersonImages.tsx around lines 30 to 33, the
renameCluster mutation currently only calls the API but doesn't update the
client cache or invalidate related queries, which leads to stale UI; modify the
mutation to use react-query's mutation callbacks (onSuccess/onSettled) to
invalidate or update relevant query keys (e.g., the cluster list and the
specific cluster detail keys) via queryClient.invalidateQueries or
queryClient.setQueryData so the UI refreshes immediately (or optimistically
update cache then confirm onSuccess), and ensure you handle errors and rollback
if using optimistic updates.
| const res: any = data?.data; | ||
| const images = (res?.images || []) as Image[]; | ||
| dispatch(setImages(images)); | ||
| setClusterName(res?.cluster_name || 'random_name'); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure payload keys align with Image type (camelCase vs snake_case)
If backend returns snake_case (e.g., thumbnail_path, cluster_name), map to the frontend Image shape or enable a global camelCase transformer. Otherwise thumbnails may not render.
Example mapping (apply where responses are unpacked):
- const fetchedImages = res.images ?? [];
+ const fetchedImages: Image[] = (res.images ?? []).map((i: any) => ({
+ id: String(i.id ?? i.image_id),
+ path: i.path,
+ thumbnailPath: i.thumbnailPath ?? i.thumbnail_path ?? i.path,
+ folder_id: i.folder_id,
+ isTagged: Boolean(i.isTagged ?? i.is_tagged),
+ metadata: i.metadata,
+ tags: i.tags,
+ bboxes: i.bboxes,
+ }));🏁 Script executed:
#!/bin/bash
# Check for snake_case fields in API responses vs camelCase in the UI
rg -n -C2 'thumbnail_path|thumbnailPath|cluster_name' frontend/src
# Look for any axios interceptor/case-conversion utility
rg -n -C2 'camel|snake|case.*convert|interceptor' frontend/srcLength of output: 4277
Map API response to the Image shape before dispatch
In frontend/src/pages/PersonImages/PersonImages.tsx (lines 42–44), you’re casting the raw res.images to Image[], but the backend returns snake_case keys (thumbnail_path, is_tagged), so properties like thumbnailPath and isTagged will be undefined. Replace the direct cast with an explicit mapping (or add a global camelCase transformer):
- const images = (res?.images || []) as Image[];
+ const images = (res?.images ?? []).map((i: any): Image => ({
+ id: String(i.id ?? i.image_id),
+ path: i.path,
+ thumbnailPath: i.thumbnailPath ?? i.thumbnail_path ?? i.path,
+ folder_id: i.folder_id,
+ isTagged: Boolean(i.isTagged ?? i.is_tagged),
+ metadata: i.metadata,
+ tags: i.tags,
+ bboxes: i.bboxes,
+ }));
dispatch(setImages(images));
setClusterName(res?.cluster_name || 'random_name');📝 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 res: any = data?.data; | |
| const images = (res?.images || []) as Image[]; | |
| dispatch(setImages(images)); | |
| setClusterName(res?.cluster_name || 'random_name'); | |
| const res: any = data?.data; | |
| const images = (res?.images ?? []).map((i: any): Image => ({ | |
| id: String(i.id ?? i.image_id), | |
| path: i.path, | |
| thumbnailPath: i.thumbnailPath ?? i.thumbnail_path ?? i.path, | |
| folder_id: i.folder_id, | |
| isTagged: Boolean(i.isTagged ?? i.is_tagged), | |
| metadata: i.metadata, | |
| tags: i.tags, | |
| bboxes: i.bboxes, | |
| })); | |
| dispatch(setImages(images)); | |
| setClusterName(res?.cluster_name || 'random_name'); |
🤖 Prompt for AI Agents
In frontend/src/pages/PersonImages/PersonImages.tsx around lines 41 to 44, the
code casts backend response images to Image[] but the backend uses snake_case
keys (e.g., thumbnail_path, is_tagged) so fields like thumbnailPath and isTagged
end up undefined; fix by explicitly mapping res.images to the Image shape
(convert snake_case keys to camelCase, provide sensible defaults, and coerce
types where needed) and then dispatch the mapped array with
setImages(mappedImages); keep setClusterName as before using res?.cluster_name
|| 'random_name'.
| const handleEditName = () => { | ||
| setClusterName(clusterName); | ||
| setIsEditing(true); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove no-op state sets; validate before saving
Calling setClusterName with the same value is redundant; also guard against empty names.
const handleEditName = () => {
- setClusterName(clusterName);
setIsEditing(true);
};
const handleSaveName = () => {
- setClusterName(clusterName);
- renameClusterMutate(clusterName);
+ const newName = clusterName.trim();
+ if (!newName) return;
+ renameClusterMutate(newName);
setIsEditing(false);
};Also applies to: 54-58
🤖 Prompt for AI Agents
In frontend/src/pages/PersonImages/PersonImages.tsx around lines 49-52 (and
similarly 54-58), remove the no-op call to setClusterName(clusterName) in the
edit handler and instead only flip the editing state; add validation in the save
handler to trim and reject empty names (or show an inline error) before calling
setClusterName(newName) and closing edit mode; ensure you only call
setClusterName when the value actually changed to avoid redundant state updates.
| return ( | ||
| <div className="flex h-full w-full items-center justify-center"> | ||
| <LoadingScreen /> | ||
| </div>; | ||
|
|
||
| return ( | ||
| <> | ||
| <MediaGallery mediaItems={videos} title="Video Gallery" type="video" /> | ||
| </> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Page stuck on a perpetual spinner
With no data flow or state change, this page will always show “Loading…”. Replace with a clear “Coming soon” or redirect; avoid indefinite loaders.
Apply:
- return (
- <div className="flex h-full w-full items-center justify-center">
- <LoadingScreen />
- </div>
- );
+ return (
+ <LoadingScreen
+ variant="fullscreen"
+ message="Videos are being revamped — coming soon."
+ isLoading
+ />
+ );📝 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.
| return ( | |
| <div className="flex h-full w-full items-center justify-center"> | |
| <LoadingScreen /> | |
| </div>; | |
| return ( | |
| <> | |
| <MediaGallery mediaItems={videos} title="Video Gallery" type="video" /> | |
| </> | |
| </div> | |
| ); | |
| return ( | |
| <LoadingScreen | |
| variant="fullscreen" | |
| message="Videos are being revamped — coming soon." | |
| isLoading | |
| /> | |
| ); |
🤖 Prompt for AI Agents
In frontend/src/pages/VideosPage/Videos.tsx around lines 4 to 8 the component
currently returns a perpetual LoadingScreen which causes the page to never
progress; replace this indefinite spinner with a deterministic outcome: either
render a centered "Coming soon" (or maintenance) message and optional
link/button back to a safe page, or perform a one-time redirect (use useNavigate
inside a useEffect to push to the desired route) so users are not stuck. Ensure
the returned JSX is static (no awaiting state) and update any related tests or
imports accordingly.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (18)
backend/tests/test_folders.py (7)
91-112: Replace tuple literals with a NamedTuple to avoid position-based brittleness.Using plain tuples makes field order easy to break. A NamedTuple preserves tuple behavior (so existing route code that indexes still works) while adding names for readability and safety.
Apply within this fixture:
@pytest.fixture def sample_folder_details(): """Sample folder details data.""" - return [ - ( - "folder-id-1", - "/home/user/photos", - None, - 1693526400, # timestamp - True, # AI_Tagging - False, # taggingCompleted - ), - ( - "folder-id-2", - "/home/user/documents", - None, - 1693526500, - False, - True, - ), - ] + from collections import namedtuple + FolderRow = namedtuple( + "FolderRow", + ["folder_id", "folder_path", "parent_folder_id", "last_scanned", "AI_Tagging", "taggingCompleted"], + ) + return [ + FolderRow("folder-id-1", "/home/user/photos", None, 1693526400, True, False), + FolderRow("folder-id-2", "/home/user/documents", None, 1693526500, False, True), + ]
321-325: Also assert executor.submit call arguments to reduce false positives.Right now we only check it was called. Lightly assert the call shape to ensure a callable and at least one arg were submitted.
app_state = client.app.state - app_state.executor.submit.assert_called_once() + app_state.executor.submit.assert_called_once() + # Basic shape checks + args, kwargs = app_state.executor.submit.call_args + assert callable(args[0]) + assert len(args) >= 1 # at least the callable
409-411: Mirror call-args assertion for enable AI background job.app_state = client.app.state - app_state.executor.submit.assert_called_once() + app_state.executor.submit.assert_called_once() + args, kwargs = app_state.executor.submit.call_args + assert callable(args[0])
515-526: Preferjson=over rawcontentfor DELETE to ensure proper encoding and headers.Using
json=is simpler and less error-prone; Starlette will set Content-Type automatically.response = client.request( "DELETE", "/folders/delete-folders", - content='{"folder_ids": ["folder-1", "folder-2", "folder-3"]}', - headers={"Content-Type": "application/json"}, + json={"folder_ids": ["folder-1", "folder-2", "folder-3"]}, )
540-546: Same: usejson=for single-folder delete.response = client.request( "DELETE", "/folders/delete-folders", - content='{"folder_ids": ["single-folder-id"]}', - headers={"Content-Type": "application/json"}, + json={"folder_ids": ["single-folder-id"]}, )
553-559: Same: usejson=for empty-list validation case.response = client.request( "DELETE", "/folders/delete-folders", - content='{"folder_ids": []}', - headers={"Content-Type": "application/json"}, + json={"folder_ids": []}, )
572-577: Same: usejson=for DB-error delete case.response = client.request( "DELETE", "/folders/delete-folders", - content='{"folder_ids": ["folder-1", "folder-2"]}', - headers={"Content-Type": "application/json"}, + json={"folder_ids": ["folder-1", "folder-2"]}, )backend/tests/test_albums.py (4)
84-88: Assert duplicate-precheck was executed.Also verify the pre-check is actually called.
mock_insert.assert_called_once() + # Ensure duplicate pre-check ran + mock_get_by_name.assert_called_once() # Verify that the album_id is a valid UUID album_id = json_response["album_id"] uuid.UUID(album_id) # This will raise ValueError if not a valid UUID
136-143: Guard against leaking password hashes in list responses.Add a negative assertion to catch accidental exposure.
assert ( json_response["albums"][0]["description"] == mock_db_album["description"] ) assert json_response["albums"][0]["is_hidden"] == mock_db_album["is_hidden"] + assert "password_hash" not in json_response["albums"][0]
216-217: Also assert sensitive fields are absent in GET-by-id.Protect against future regressions exposing password hashes.
assert json_response["data"]["description"] == mock_db_album["description"] assert json_response["data"]["is_hidden"] == mock_db_album["is_hidden"] + assert "password_hash" not in json_response["data"]
71-74: Explicitly set 201 status for POST /albums/
The route decorator currently omitsstatus_code, so FastAPI defaults to 200 (and the test rightly asserts 200). If you intend RESTful “created” semantics, addstatus_code=status.HTTP_201_CREATEDto@router.post(...)and update the test to expect 201.backend/tests/test_face_clusters.py (7)
38-56: Fixture LGTM; ensure schema parity across tests.The added face_image_base64 entries look good. Make sure all schema-level tests also assert its presence/type so regressions are caught (see suggestion on Lines 283-291).
101-119: Avoid brittle message assertions.Asserting substrings in human-readable messages is fragile. Prefer structural assertions (status, ids, updated names). Consider dropping the message assertion here.
- assert "Successfully renamed cluster" in data["message"]
127-138: Include identifier in error response for better DX.Consider asserting that detail.message mentions the cluster_id to aid debugging. If not present, consider adding it in the route.
152-164: Edge case covered; path whitespace is unusual.Good to test, but whitespace in path params is uncommon. Consider adding a test for URL-encoded whitespace to ensure consistent handling.
165-183: Also test when update raises an exception.You cover False return; add a case where db_update_cluster raises to ensure 500 path is exercised.
Example test to add:
@patch("app.routes.face_clusters.db_update_cluster") @patch("app.routes.face_clusters.db_get_cluster_by_id") def test_rename_cluster_update_raises(self, mock_get, mock_update): mock_get.return_value = {"cluster_id": "cluster_123", "cluster_name": "Old Name"} mock_update.side_effect = Exception("write failed") resp = client.put("/face_clusters/cluster_123", json={"cluster_name": "New"}) assert resp.status_code == 500 data = resp.json() assert data["detail"]["error"] == "Internal server error"
283-291: Strengthen structure checks to include preview field.Add face_image_base64 to required cluster fields and type-check.
Apply this diff:
- assert "clusters" in response_data["data"] - for cluster in response_data["data"]["clusters"]: - cluster_fields = ["cluster_id", "cluster_name", "face_count"] + assert "clusters" in response_data["data"] + for cluster in response_data["data"]["clusters"]: + cluster_fields = ["cluster_id", "cluster_name", "face_count", "face_image_base64"] for field in cluster_fields: assert field in cluster @@ assert isinstance(cluster["cluster_id"], str) assert isinstance(cluster["cluster_name"], str) assert isinstance(cluster["face_count"], int) + assert isinstance(cluster["face_image_base64"], str)Also applies to: 297-297
92-95: Extract and use a shared TestClient fixture instead of a module‐level client.Move the existing
@pytest.fixture def client(app_with_state): return TestClient(app_with_state)from tests/test_folders.py into tests/conftest.py so it’s available to all test modules. In test_face_clusters.py, drop the top-level app/TestClient instantiation and add
client(and, if needed,app_with_state) as test parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
backend/tests/test_albums.py(5 hunks)backend/tests/test_face_clusters.py(4 hunks)backend/tests/test_folders.py(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/test_face_clusters.py (1)
backend/tests/test_folders.py (1)
client(76-78)
⏰ 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 Tests
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
🔇 Additional comments (16)
backend/tests/test_folders.py (7)
158-160: Good: asserts validate new data envelope for add-folder.
254-254: Good: verifies folder_id propagation when parent ID provided.
343-345: Good: updated_count and folder_ids assertions match new response schema.
432-439: Good: disable AI tagging response shape checks look correct.
457-457: Good: single-folder disable path verified.
672-673: Good: partial-update path covered.
688-689: Good: zero-updates path covered.backend/tests/test_albums.py (1)
7-7: LGTM: added unittest.mock.patch import.backend/tests/test_face_clusters.py (8)
139-151: 422 vs 400 for validation errors.If Pydantic validation is enforcing non-empty strings, 422 Unprocessable Entity is more idiomatic. If validation is manual, 400 is acceptable—just be consistent across endpoints.
184-197: LGTM: database exception path tested.
198-218: LGTM: trims whitespace and asserts trimmed value is persisted.
249-260: LGTM: empty state covered well.
262-272: LGTM: database error path for list clusters.
335-347: LGTM: 404 path and error shape asserted.
348-367: LGTM: empty images state covered.
368-380: LGTM: images DB error path covered.
| def test_create_album_duplicate_name(self): | ||
| """Test creating album with duplicate name.""" | ||
| album_data = { | ||
| "name": "Existing Album", | ||
| "description": "This name already exists", | ||
| "is_hidden": False, | ||
| "password": None, | ||
| } | ||
|
|
||
| with patch("app.routes.albums.db_get_album_by_name") as mock_get_by_name: | ||
| mock_get_by_name.return_value = ( | ||
| "existing-id", | ||
| "Existing Album", | ||
| "desc", | ||
| 0, | ||
| None, | ||
| ) | ||
|
|
||
| response = client.post("/albums/", json=album_data) | ||
| assert response.status_code == 409 | ||
|
|
||
| json_response = response.json() | ||
| assert json_response["detail"]["success"] is False | ||
| assert json_response["detail"]["error"] == "Album Already Exists" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen duplicate-name test: ensure no insert and use realistic UUID.
Prevent regressions by asserting insert isn’t called on duplicates and prefer a UUID for the mocked existing record. Optionally assert presence of a human-readable message for consistency with other errors.
- with patch("app.routes.albums.db_get_album_by_name") as mock_get_by_name:
+ with patch("app.routes.albums.db_get_album_by_name") as mock_get_by_name, \
+ patch("app.routes.albums.db_insert_album") as mock_insert:
mock_get_by_name.return_value = (
- "existing-id",
+ str(uuid.uuid4()),
"Existing Album",
"desc",
0,
None,
)
response = client.post("/albums/", json=album_data)
assert response.status_code == 409
json_response = response.json()
assert json_response["detail"]["success"] is False
assert json_response["detail"]["error"] == "Album Already Exists"
+ # Optional consistency check with other error responses
+ assert "message" in json_response["detail"]
+ # Ensure no write occurs when duplicate is detected
+ mock_insert.assert_not_called()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/tests/test_albums.py around lines 89 to 113, the duplicate-name test
should be strengthened: update the mocked existing record id to a realistic UUID
(e.g. "123e4567-e89b-12d3-a456-426614174000"), add an assertion that the album
insert function (patch the insert/save function used by the route) was not
called when a duplicate is detected, and optionally assert that the JSON error
includes a human-readable message key (e.g. checks
json_response["detail"].get("message") or similar) to keep consistency with
other error responses.
| @pytest.fixture | ||
| def sample_cluster_images(): | ||
| """Sample images data for a cluster.""" | ||
| return [ | ||
| { | ||
| "image_id": "img_1", | ||
| "image_path": "/path/to/image1.jpg", | ||
| "thumbnail_path": "/path/to/thumb1.jpg", | ||
| "metadata": "{'camera': 'Canon'}", | ||
| "face_id": 101, | ||
| "confidence": 0.95, | ||
| "bbox": {"x": 100, "y": 200, "width": 150, "height": 200}, | ||
| }, | ||
| { | ||
| "image_id": "img_2", | ||
| "image_path": "/path/to/image2.jpg", | ||
| "thumbnail_path": "/path/to/thumb2.jpg", | ||
| "metadata": "{'camera': 'Nikon'}", | ||
| "face_id": 102, | ||
| "confidence": 0.87, | ||
| "bbox": {"x": 50, "y": 100, "width": 120, "height": 160}, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use structured metadata, not a string.
metadata should be a JSON object/dict to match typical API schemas and avoid downstream parsing. Update the fixture to dicts.
Apply this diff:
@@
{
"image_id": "img_1",
"image_path": "/path/to/image1.jpg",
"thumbnail_path": "/path/to/thumb1.jpg",
- "metadata": "{'camera': 'Canon'}",
+ "metadata": {"camera": "Canon"},
"face_id": 101,
"confidence": 0.95,
"bbox": {"x": 100, "y": 200, "width": 150, "height": 200},
},
{
"image_id": "img_2",
"image_path": "/path/to/image2.jpg",
"thumbnail_path": "/path/to/thumb2.jpg",
- "metadata": "{'camera': 'Nikon'}",
+ "metadata": {"camera": "Nikon"},
"face_id": 102,
"confidence": 0.87,
"bbox": {"x": 50, "y": 100, "width": 120, "height": 160},
},📝 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.
| @pytest.fixture | |
| def sample_cluster_images(): | |
| """Sample images data for a cluster.""" | |
| return [ | |
| { | |
| "image_id": "img_1", | |
| "image_path": "/path/to/image1.jpg", | |
| "thumbnail_path": "/path/to/thumb1.jpg", | |
| "metadata": "{'camera': 'Canon'}", | |
| "face_id": 101, | |
| "confidence": 0.95, | |
| "bbox": {"x": 100, "y": 200, "width": 150, "height": 200}, | |
| }, | |
| { | |
| "image_id": "img_2", | |
| "image_path": "/path/to/image2.jpg", | |
| "thumbnail_path": "/path/to/thumb2.jpg", | |
| "metadata": "{'camera': 'Nikon'}", | |
| "face_id": 102, | |
| "confidence": 0.87, | |
| "bbox": {"x": 50, "y": 100, "width": 120, "height": 160}, | |
| }, | |
| @pytest.fixture | |
| def sample_cluster_images(): | |
| """Sample images data for a cluster.""" | |
| return [ | |
| { | |
| "image_id": "img_1", | |
| "image_path": "/path/to/image1.jpg", | |
| "thumbnail_path": "/path/to/thumb1.jpg", | |
| "metadata": {"camera": "Canon"}, | |
| "face_id": 101, | |
| "confidence": 0.95, | |
| "bbox": {"x": 100, "y": 200, "width": 150, "height": 200}, | |
| }, | |
| { | |
| "image_id": "img_2", | |
| "image_path": "/path/to/image2.jpg", | |
| "thumbnail_path": "/path/to/thumb2.jpg", | |
| "metadata": {"camera": "Nikon"}, | |
| "face_id": 102, | |
| "confidence": 0.87, | |
| "bbox": {"x": 50, "y": 100, "width": 120, "height": 160}, | |
| }, | |
| ] |
🤖 Prompt for AI Agents
In backend/tests/test_face_clusters.py around lines 59 to 80, the fixture uses
metadata as a string instead of a dict; change each metadata value from a string
like "{'camera': 'Canon'}" to an actual dict {'camera': 'Canon'} (and similarly
for Nikon) so the fixture returns structured JSON-like metadata objects matching
the API schema and avoiding downstream parsing issues.
| data = response.json() | ||
| assert data["success"] is True | ||
| assert "Successfully retrieved 3 cluster(s)" in data["message"] | ||
| assert len(data["data"]["clusters"]) == 3 | ||
|
|
||
| # Check first cluster details | ||
| first_cluster = data["data"]["clusters"][0] | ||
| assert first_cluster["cluster_id"] == "cluster_1" | ||
| assert first_cluster["cluster_name"] == "John Doe" | ||
| assert first_cluster["face_count"] == 15 | ||
| assert first_cluster["face_image_base64"] == "base64_string_1" | ||
|
|
||
| mock_get_clusters.assert_called_once() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t couple to list order; assert by id.
Order isn’t guaranteed unless the API specifies it. Match clusters by id to avoid flaky tests.
Apply this diff:
- # Check first cluster details
- first_cluster = data["data"]["clusters"][0]
- assert first_cluster["cluster_id"] == "cluster_1"
- assert first_cluster["cluster_name"] == "John Doe"
- assert first_cluster["face_count"] == 15
- assert first_cluster["face_image_base64"] == "base64_string_1"
+ clusters = data["data"]["clusters"]
+ john = next(c for c in clusters if c["cluster_id"] == "cluster_1")
+ assert john["cluster_name"] == "John Doe"
+ assert john["face_count"] == 15
+ assert john["face_image_base64"] == "base64_string_1"📝 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.
| data = response.json() | |
| assert data["success"] is True | |
| assert "Successfully retrieved 3 cluster(s)" in data["message"] | |
| assert len(data["data"]["clusters"]) == 3 | |
| # Check first cluster details | |
| first_cluster = data["data"]["clusters"][0] | |
| assert first_cluster["cluster_id"] == "cluster_1" | |
| assert first_cluster["cluster_name"] == "John Doe" | |
| assert first_cluster["face_count"] == 15 | |
| assert first_cluster["face_image_base64"] == "base64_string_1" | |
| mock_get_clusters.assert_called_once() | |
| data = response.json() | |
| assert data["success"] is True | |
| assert "Successfully retrieved 3 cluster(s)" in data["message"] | |
| assert len(data["data"]["clusters"]) == 3 | |
| clusters = data["data"]["clusters"] | |
| john = next(c for c in clusters if c["cluster_id"] == "cluster_1") | |
| assert john["cluster_name"] == "John Doe" | |
| assert john["face_count"] == 15 | |
| assert john["face_image_base64"] == "base64_string_1" | |
| mock_get_clusters.assert_called_once() |
🤖 Prompt for AI Agents
In backend/tests/test_face_clusters.py around lines 234 to 247, the test
currently assumes a fixed list order for clusters which can be flaky; instead,
keep the length assertion but locate clusters by their cluster_id (e.g., build a
dict mapping cluster_id -> cluster) and then assert each expected cluster's
fields (cluster_name, face_count, face_image_base64) by looking them up by id;
leave the mock_get_clusters.assert_called_once() intact.
| @patch("app.routes.face_clusters.db_get_images_by_cluster_id") | ||
| @patch("app.routes.face_clusters.db_get_cluster_by_id") | ||
| def test_get_cluster_images_success( | ||
| self, mock_get_cluster, mock_get_images, sample_cluster_images | ||
| ): | ||
| """Test successfully retrieving images for a cluster.""" | ||
| cluster_id = "cluster_123" | ||
| mock_get_cluster.return_value = { | ||
| "cluster_id": cluster_id, | ||
| "cluster_name": "John Doe", | ||
| } | ||
| mock_get_images.return_value = sample_cluster_images | ||
|
|
||
| response = client.get(f"/face_clusters/{cluster_id}/images") | ||
|
|
||
| assert response.status_code == 200 | ||
| data = response.json() | ||
| assert data["success"] is True | ||
| assert "Successfully retrieved 2 image(s)" in data["message"] | ||
| assert data["data"]["cluster_id"] == cluster_id | ||
| assert data["data"]["cluster_name"] == "John Doe" | ||
| assert data["data"]["total_images"] == 2 | ||
| assert len(data["data"]["images"]) == 2 | ||
|
|
||
| # Check first image details | ||
| first_image = data["data"]["images"][0] | ||
| assert first_image["id"] == "img_1" | ||
| assert first_image["path"] == "/path/to/image1.jpg" | ||
| assert first_image["face_id"] == 101 | ||
| assert first_image["confidence"] == 0.95 | ||
|
|
||
| mock_get_cluster.assert_called_once_with(cluster_id) | ||
| mock_get_images.assert_called_once_with(cluster_id) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Assert all image fields and use approx for floats.
Cover thumbnail_path, metadata type, bbox shape/types; use approx for confidence to avoid float pitfalls.
Apply this diff:
- assert first_image["id"] == "img_1"
- assert first_image["path"] == "/path/to/image1.jpg"
- assert first_image["face_id"] == 101
- assert first_image["confidence"] == 0.95
+ assert first_image["id"] == "img_1"
+ assert first_image["path"] == "/path/to/image1.jpg"
+ assert first_image["thumbnail_path"] == "/path/to/thumb1.jpg"
+ assert isinstance(first_image["metadata"], dict)
+ assert first_image["metadata"].get("camera") == "Canon"
+ assert first_image["face_id"] == 101
+ assert first_image["confidence"] == pytest.approx(0.95, rel=1e-6)
+ assert set(first_image["bbox"].keys()) == {"x", "y", "width", "height"}
+ assert all(isinstance(first_image["bbox"][k], int) for k in ["x", "y", "width", "height"])If the public schema uses a different field name (e.g., thumbnail instead of thumbnail_path), align the assertion accordingly.
🏁 Script executed:
#!/bin/bash
# Inspect schema to confirm image field names
rg -nC2 -P 'class\s+ImageInCluster|thumbnail_?path|thumbnail\s*:' backend/app/schemas/face_clusters.pyLength of output: 230
🏁 Script executed:
#!/bin/bash
# Print fields inside ImageInCluster schema class
sed -n '45,80p' backend/app/schemas/face_clusters.pyLength of output: 818
🏁 Script executed:
rg -nC3 "sample_cluster_images" backend/tests/test_face_clusters.pyLength of output: 830
🏁 Script executed:
#!/bin/bash
sed -n '60,120p' backend/tests/test_face_clusters.pyLength of output: 2202
Assert all image fields with correct names and use approx for floats.
Cover thumbnailPath, metadata as a string (contains "Canon"), bbox keys/types; use pytest.approx for confidence.
- assert first_image["id"] == "img_1"
- assert first_image["path"] == "/path/to/image1.jpg"
- assert first_image["face_id"] == 101
- assert first_image["confidence"] == 0.95
+ assert first_image["id"] == "img_1"
+ assert first_image["path"] == "/path/to/image1.jpg"
+ assert first_image["thumbnailPath"] == "/path/to/thumb1.jpg"
+ assert isinstance(first_image["metadata"], str)
+ assert "Canon" in first_image["metadata"]
+ assert first_image["face_id"] == 101
+ assert first_image["confidence"] == pytest.approx(0.95, rel=1e-6)
+ assert set(first_image["bbox"].keys()) == {"x", "y", "width", "height"}
+ assert all(isinstance(v, (int, float)) for v in first_image["bbox"].values())📝 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.
| @patch("app.routes.face_clusters.db_get_images_by_cluster_id") | |
| @patch("app.routes.face_clusters.db_get_cluster_by_id") | |
| def test_get_cluster_images_success( | |
| self, mock_get_cluster, mock_get_images, sample_cluster_images | |
| ): | |
| """Test successfully retrieving images for a cluster.""" | |
| cluster_id = "cluster_123" | |
| mock_get_cluster.return_value = { | |
| "cluster_id": cluster_id, | |
| "cluster_name": "John Doe", | |
| } | |
| mock_get_images.return_value = sample_cluster_images | |
| response = client.get(f"/face_clusters/{cluster_id}/images") | |
| assert response.status_code == 200 | |
| data = response.json() | |
| assert data["success"] is True | |
| assert "Successfully retrieved 2 image(s)" in data["message"] | |
| assert data["data"]["cluster_id"] == cluster_id | |
| assert data["data"]["cluster_name"] == "John Doe" | |
| assert data["data"]["total_images"] == 2 | |
| assert len(data["data"]["images"]) == 2 | |
| # Check first image details | |
| first_image = data["data"]["images"][0] | |
| assert first_image["id"] == "img_1" | |
| assert first_image["path"] == "/path/to/image1.jpg" | |
| assert first_image["face_id"] == 101 | |
| assert first_image["confidence"] == 0.95 | |
| mock_get_cluster.assert_called_once_with(cluster_id) | |
| mock_get_images.assert_called_once_with(cluster_id) | |
| @patch("app.routes.face_clusters.db_get_images_by_cluster_id") | |
| @patch("app.routes.face_clusters.db_get_cluster_by_id") | |
| def test_get_cluster_images_success( | |
| self, mock_get_cluster, mock_get_images, sample_cluster_images | |
| ): | |
| """Test successfully retrieving images for a cluster.""" | |
| cluster_id = "cluster_123" | |
| mock_get_cluster.return_value = { | |
| "cluster_id": cluster_id, | |
| "cluster_name": "John Doe", | |
| } | |
| mock_get_images.return_value = sample_cluster_images | |
| response = client.get(f"/face_clusters/{cluster_id}/images") | |
| assert response.status_code == 200 | |
| data = response.json() | |
| assert data["success"] is True | |
| assert "Successfully retrieved 2 image(s)" in data["message"] | |
| assert data["data"]["cluster_id"] == cluster_id | |
| assert data["data"]["cluster_name"] == "John Doe" | |
| assert data["data"]["total_images"] == 2 | |
| assert len(data["data"]["images"]) == 2 | |
| # Check first image details | |
| first_image = data["data"]["images"][0] | |
| assert first_image["id"] == "img_1" | |
| assert first_image["path"] == "/path/to/image1.jpg" | |
| assert first_image["thumbnailPath"] == "/path/to/thumb1.jpg" | |
| assert isinstance(first_image["metadata"], str) | |
| assert "Canon" in first_image["metadata"] | |
| assert first_image["face_id"] == 101 | |
| assert first_image["confidence"] == pytest.approx(0.95, rel=1e-6) | |
| assert set(first_image["bbox"].keys()) == {"x", "y", "width", "height"} | |
| assert all(isinstance(v, (int, float)) for v in first_image["bbox"].values()) | |
| mock_get_cluster.assert_called_once_with(cluster_id) | |
| mock_get_images.assert_called_once_with(cluster_id) |
🤖 Prompt for AI Agents
In backend/tests/test_face_clusters.py around lines 301 to 334, update the test
assertions to verify all expected image fields and types: assert the image dict
contains "thumbnailPath" (correct camelCase) and that "metadata" is a string
containing "Canon"; assert the "bbox" object has the expected keys ("x", "y",
"w", "h") and that their values are numeric types (int/float); replace direct
float equality for "confidence" with pytest.approx to allow small tolerance;
ensure pytest is imported if not already and adjust assertion order to use these
exact field names and checks.
| @patch("app.routes.folders.db_get_all_folder_details") | ||
| def test_get_all_folders_success( | ||
| self, mock_get_all_folders, client, sample_folder_details | ||
| ): | ||
| """Test successfully retrieving all folders.""" | ||
| mock_get_all_folders.return_value = sample_folder_details | ||
|
|
||
| response = client.get("/folders/all-folders") | ||
|
|
||
| assert response.status_code == 200 | ||
| data = response.json() | ||
| assert data["success"] is True | ||
| assert "Successfully retrieved 2 folder(s)" in data["message"] | ||
| assert data["data"]["total_count"] == 2 | ||
| assert len(data["data"]["folders"]) == 2 | ||
|
|
||
| # Check first folder details | ||
| first_folder = data["data"]["folders"][0] | ||
| assert first_folder["folder_id"] == "folder-id-1" | ||
| assert first_folder["folder_path"] == "/home/user/photos" | ||
| assert first_folder["parent_folder_id"] is None | ||
| assert first_folder["AI_Tagging"] is True | ||
| assert first_folder["taggingCompleted"] is False | ||
|
|
||
| mock_get_all_folders.assert_called_once() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid assuming DB/order of folders; assert by ID to prevent flakiness.
Current test indexes element 0. If backend changes sort order, this will flake.
# Check first folder details
-first_folder = data["data"]["folders"][0]
-assert first_folder["folder_id"] == "folder-id-1"
-assert first_folder["folder_path"] == "/home/user/photos"
-assert first_folder["parent_folder_id"] is None
-assert first_folder["AI_Tagging"] is True
-assert first_folder["taggingCompleted"] is False
+by_id = {f["folder_id"]: f for f in data["data"]["folders"]}
+f1 = by_id["folder-id-1"]
+assert f1["folder_path"] == "/home/user/photos"
+assert f1["parent_folder_id"] is None
+assert f1["AI_Tagging"] is True
+assert f1["taggingCompleted"] is False📝 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.
| @patch("app.routes.folders.db_get_all_folder_details") | |
| def test_get_all_folders_success( | |
| self, mock_get_all_folders, client, sample_folder_details | |
| ): | |
| """Test successfully retrieving all folders.""" | |
| mock_get_all_folders.return_value = sample_folder_details | |
| response = client.get("/folders/all-folders") | |
| assert response.status_code == 200 | |
| data = response.json() | |
| assert data["success"] is True | |
| assert "Successfully retrieved 2 folder(s)" in data["message"] | |
| assert data["data"]["total_count"] == 2 | |
| assert len(data["data"]["folders"]) == 2 | |
| # Check first folder details | |
| first_folder = data["data"]["folders"][0] | |
| assert first_folder["folder_id"] == "folder-id-1" | |
| assert first_folder["folder_path"] == "/home/user/photos" | |
| assert first_folder["parent_folder_id"] is None | |
| assert first_folder["AI_Tagging"] is True | |
| assert first_folder["taggingCompleted"] is False | |
| mock_get_all_folders.assert_called_once() | |
| @patch("app.routes.folders.db_get_all_folder_details") | |
| def test_get_all_folders_success( | |
| self, mock_get_all_folders, client, sample_folder_details | |
| ): | |
| """Test successfully retrieving all folders.""" | |
| mock_get_all_folders.return_value = sample_folder_details | |
| response = client.get("/folders/all-folders") | |
| assert response.status_code == 200 | |
| data = response.json() | |
| assert data["success"] is True | |
| assert "Successfully retrieved 2 folder(s)" in data["message"] | |
| assert data["data"]["total_count"] == 2 | |
| assert len(data["data"]["folders"]) == 2 | |
| # Check first folder details | |
| by_id = {f["folder_id"]: f for f in data["data"]["folders"]} | |
| f1 = by_id["folder-id-1"] | |
| assert f1["folder_path"] == "/home/user/photos" | |
| assert f1["parent_folder_id"] is None | |
| assert f1["AI_Tagging"] is True | |
| assert f1["taggingCompleted"] is False | |
| mock_get_all_folders.assert_called_once() |
🤖 Prompt for AI Agents
In backend/tests/test_folders.py around lines 588 to 613, the test assumes a
stable ordering by indexing into folders[0], which can flake if DB/order
changes; instead locate the folder object by its folder_id and assert its
fields. Update the test to search the returned folders list for the entry with
folder_id == "folder-id-1" (fail the test if not found) and then assert
folder_path, parent_folder_id, AI_Tagging, and taggingCompleted on that found
object, keeping the existing assertions on total_count and length intact and
still verifying mock_get_all_folders was called once.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/tests/test_folders.py (1)
588-595: Avoid order-dependent assertions in list responsesIndexing element 0 makes this test flaky if backend sort changes. Assert by ID.
Apply:
- # Check first folder details - first_folder = data["data"]["folders"][0] - assert first_folder["folder_id"] == "folder-id-1" - assert first_folder["folder_path"] == "/home/user/photos" - assert first_folder["parent_folder_id"] is None - assert first_folder["AI_Tagging"] is True - assert first_folder["taggingCompleted"] is False + by_id = {f["folder_id"]: f for f in data["data"]["folders"]} + f1 = by_id["folder-id-1"] + assert f1["folder_path"] == "/home/user/photos" + assert f1["parent_folder_id"] is None + assert f1["AI_Tagging"] is True + assert f1["taggingCompleted"] is False
🧹 Nitpick comments (9)
backend/tests/test_folders.py (2)
505-513: Prefer json= with DELETE for clarity and type-safetyUsing json= avoids manual JSON strings and mismatched headers.
- response = client.request( - "DELETE", - "/folders/delete-folders", - content='{"folder_ids": ["folder-1", "folder-2", "folder-3"]}', - headers={"Content-Type": "application/json"}, - ) + response = client.delete( + "/folders/delete-folders", + json={"folder_ids": ["folder-1", "folder-2", "folder-3"]}, + )(Apply similarly in the other DELETE tests in this block.)
Also applies to: 526-535, 540-548, 558-566
315-319: Strengthen background job assertionsAlso assert that a callable is submitted to the executor to catch regressions in submit args.
- app_state.executor.submit.assert_called_once() + app_state.executor.submit.assert_called_once() + (args, kwargs) = app_state.executor.submit.call_args + assert callable(args[0]), "submit() should be called with a callable as first arg"Also applies to: 401-404
docs/backend/backend_python/openapi.json (7)
1-18: Add authentication to the spec (securitySchemes + global security)Spec lacks any security; if endpoints aren’t public, define a scheme and apply globally.
"servers": [ { "url": "http://localhost:8000", "description": "Local Development server" } ], + "security": [ + { "BearerAuth": [] } + ], "paths": { @@ "components": { - "schemas": { + "securitySchemes": { + "BearerAuth": { + "type": "http", + "scheme": "bearer", + "bearerFormat": "JWT" + } + }, + "schemas": {Note: If the API is intentionally unauthenticated, consider documenting that explicitly and suppressing scanner rules in CI.
Also applies to: 1131-1134
1295-1299: Resolve ambiguity: face_image_base64 required yet nullableCurrently listed in required but allows null, which confuses clients.
- "required": [ - "cluster_id", - "cluster_name", - "face_image_base64", - "face_count" - ], + "required": [ + "cluster_id", + "cluster_name", + "face_count" + ],Or disallow null by removing null from anyOf if it must always be present.
Also applies to: 1277-1287
1384-1398: Bound request array sizes and enforce uniquenessAdd minItems/maxItems/uniqueItems to mitigate abuse and align with lint rules.
"DeleteFoldersRequest": { "properties": { "folder_ids": { "items": { "type": "string" }, "type": "array", - "title": "Folder Ids" + "title": "Folder Ids", + "minItems": 1, + "maxItems": 1000, + "uniqueItems": true } }, @@ "UpdateAITaggingRequest": { "properties": { "folder_ids": { "items": { "type": "string" }, "type": "array", - "title": "Folder Ids" + "title": "Folder Ids", + "minItems": 1, + "maxItems": 1000, + "uniqueItems": true } }, @@ "ImageIdsRequest": { "properties": { "image_ids": { "items": { "type": "string" }, "type": "array", - "title": "Image Ids" + "title": "Image Ids", + "minItems": 1, + "maxItems": 1000, + "uniqueItems": true } },Also applies to: 2212-2226, 1896-1910
1958-1978: Specify bbox shape explicitlyTyped object properties improve client generation and validation.
- "bbox": { - "anyOf": [ - { - "additionalProperties": { - "anyOf": [{ "type": "integer" }, { "type": "number" }] - }, - "type": "object" - }, - { "type": "null" } - ], - "title": "Bbox" - } + "bbox": { + "type": "object", + "properties": { + "x": { "type": "number" }, + "y": { "type": "number" }, + "width": { "type": "number" }, + "height": { "type": "number" } + }, + "required": ["x", "y", "width", "height"], + "title": "Bbox" + }
1637-1661: Make message optional to match other envelopesOther responses use optional message/error; align here for consistency.
"GetAllImagesResponse": { "properties": { "success": { "type": "boolean", "title": "Success" }, - "message": { "type": "string", "title": "Message" }, + "message": { + "anyOf": [{ "type": "string" }, { "type": "null" }], + "title": "Message" + }, "data": { "items": { "$ref": "#/components/schemas/ImageData" }, "type": "array", "title": "Data" } }, "type": "object", - "required": ["success", "message", "data"], + "required": ["success", "data"], "title": "GetAllImagesResponse" },
2469-2501: Unify error envelope shapes across domainsFolders/FaceClusters errors allow nullable message/error with success default false; Images/UserPreferences differ. Standardize to one pattern to simplify frontend handling.
"app__schemas__images__ErrorResponse": { "properties": { - "success": { "type": "boolean", "title": "Success", "default": false }, - "message": { "type": "string", "title": "Message" }, - "error": { "type": "string", "title": "Error" } + "success": { "type": "boolean", "title": "Success", "default": false }, + "message": { "anyOf": [{ "type": "string" }, { "type": "null" }], "title": "Message" }, + "error": { "anyOf": [{ "type": "string" }, { "type": "null" }], "title": "Error" } }, "type": "object", - "required": ["message", "error"], + "required": [], "title": "ErrorResponse" },(Apply the same pattern to app__schemas__user_preferences__ErrorResponse if desired.)
Also applies to: 2502-2524
377-408: Normalize trailing slashes in pathsSome endpoints include a trailing slash (/images/, /face-clusters/) while others don’t (/folders/all-folders). Pick one convention to avoid client/router inconsistencies.
Also applies to: 828-859
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
backend/main.py(5 hunks)backend/tests/test_folders.py(15 hunks)docs/backend/backend_python/openapi.json(22 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/main.py
🧰 Additional context used
🪛 Checkov (3.2.334)
docs/backend/backend_python/openapi.json
[MEDIUM] 1368-1374: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
[HIGH] 1-2550: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[HIGH] 1-2550: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
⏰ 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 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 (2)
backend/tests/test_folders.py (2)
91-112: LGTM: Solid sample data fixtureRepresentative fields and types match the envelope response used in tests.
722-737: Double-check patch argument orderDecorator stacking reverses arg order; current signature looks correct (inner -> first arg). Keep as-is, but please confirm locally to avoid swapped mocks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/tests/test_folders.py (1)
604-611: Don’t assume folder ordering; assert by ID for stabilityIndexing the first element will flake if DB sort changes. Build a map and assert by key.
- # Check first folder details - first_folder = data["data"]["folders"][0] - assert first_folder["folder_id"] == "folder-id-1" - assert first_folder["folder_path"] == "/home/user/photos" - assert first_folder["parent_folder_id"] is None - assert first_folder["AI_Tagging"] is True - assert first_folder["taggingCompleted"] is False + # Assert by ID to avoid order-dependence + by_id = {f["folder_id"]: f for f in data["data"]["folders"]} + f1 = by_id["folder-id-1"] + assert f1["folder_path"] == "/home/user/photos" + assert f1["parent_folder_id"] is None + assert f1["AI_Tagging"] is True + assert f1["taggingCompleted"] is False
🧹 Nitpick comments (4)
backend/tests/test_folders.py (4)
91-112: Make tuple construction explicit to prevent positional mistakesThe fixture returns raw tuples; a small helper keeps field order self-documented and reduces slip-ups.
@pytest.fixture def sample_folder_details(): """Sample folder details data.""" - return [ - ( - "folder-id-1", - "/home/user/photos", - None, - 1693526400, # timestamp - True, # AI_Tagging - False, # taggingCompleted - ), - ( - "folder-id-2", - "/home/user/documents", - None, - 1693526500, - False, - True, - ), - ] + def row(folder_id, folder_path, parent_folder_id, ts, ai_tagging, tagging_completed): + # (folder_id, folder_path, parent_folder_id, created_at_ts, AI_Tagging, taggingCompleted) + return (folder_id, folder_path, parent_folder_id, ts, ai_tagging, tagging_completed) + + return [ + row("folder-id-1", "/home/user/photos", None, 1693526400, True, False), + row("folder-id-2", "/home/user/documents", None, 1693526500, False, True), + ]
519-524: Prefer client.delete(..., json=...) over manual content + headersThis is simpler, avoids typos, and mirrors how other tests pass JSON.
- response = client.request( - "DELETE", - "/folders/delete-folders", - content='{"folder_ids": ["folder-1", "folder-2", "folder-3"]}', - headers={"Content-Type": "application/json"}, - ) + response = client.delete( + "/folders/delete-folders", + json={"folder_ids": ["folder-1", "folder-2", "folder-3"]}, + )- response = client.request( - "DELETE", - "/folders/delete-folders", - content='{"folder_ids": ["single-folder-id"]}', - headers={"Content-Type": "application/json"}, - ) + response = client.delete( + "/folders/delete-folders", + json={"folder_ids": ["single-folder-id"]}, + )- response = client.request( - "DELETE", - "/folders/delete-folders", - content='{"folder_ids": []}', - headers={"Content-Type": "application/json"}, - ) + response = client.delete( + "/folders/delete-folders", + json={"folder_ids": []}, + )- response = client.request( - "DELETE", - "/folders/delete-folders", - content='{"folder_ids": ["folder-1", "folder-2"]}', - headers={"Content-Type": "application/json"}, - ) + response = client.delete( + "/folders/delete-folders", + json={"folder_ids": ["folder-1", "folder-2"]}, + )- delete_response = client.request( - "DELETE", - "/folders/delete-folders", - content='{"folder_ids": ["folder-1", "folder-2"]}', - headers={"Content-Type": "application/json"}, - ) + delete_response = client.delete( + "/folders/delete-folders", + json={"folder_ids": ["folder-1", "folder-2"]}, + )Also applies to: 540-545, 555-559, 572-577, 825-830
275-281: Assert no background work on error pathsTo fully specify behavior, ensure executor.submit is not called when the endpoint fails.
assert response.status_code == 500 data = response.json() assert data["detail"]["success"] is False assert data["detail"]["error"] == "Internal server error" + # No background processing on failure + app_state = client.app.state + app_state.executor.submit.assert_not_called()Also applies to: 391-395, 488-492
321-325: Optional: validate submit arguments, not just call countIf feasible, assert the task function and arguments (e.g., folder_id(s)) passed to executor.submit to catch wiring errors.
Also applies to: 409-412
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/main.py(4 hunks)backend/tests/test_folders.py(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/main.py
⏰ 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 (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Tests
🔇 Additional comments (1)
backend/tests/test_folders.py (1)
158-160: Envelope migration assertions look correctGood job updating tests to assert fields under data while preserving messages and success flags.
Also applies to: 254-254, 343-345, 360-360, 432-439, 457-457, 672-672, 688-688, 727-727, 733-733, 752-752, 759-759, 832-832
2 participants
Implemented the new frontend:
https://pictopy-new-frontend.vercel.app/
Also fixes the issue: #473
Summary by CodeRabbit
New Features
Improvements
Removals
Chores