feat: add global face reclustering option in settings with backend API and UI support #560
Conversation
WalkthroughAdds a manual global face reclustering flow: backend POST /face-clusters/global-recluster endpoint and response schemas, a force-capable reclustering util path, frontend API/endpoint, and a "Recluster Faces" button in Settings wired to a mutation and feedback. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Settings UI
participant API as Frontend API
participant Backend as Backend Route
participant Util as Clustering Util
participant DB as Database
User->>UI: Click "Recluster Faces"
UI->>API: triggerGlobalReclustering()
API->>Backend: POST /face-clusters/global-recluster
Backend->>Util: cluster_util_face_clusters_sync(force_full_reclustering=True)
Util->>DB: Query faces & compute clusters
alt Faces Found
Util->>DB: Delete existing clusters
Util->>DB: Batch insert new clusters
Util->>DB: Batch update face cluster IDs
Util->>DB: Generate/update representative images
Util->>Backend: return clusters_created
else No Faces
Util->>Backend: return 0
end
Backend->>API: GlobalReclusterResponse { success, message, data: { clusters_created } }
API->>UI: Response
UI->>UI: Show feedback, invalidate 'clusters' tag
UI->>User: Display result message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/app/database/face_clusters.py(1 hunks)backend/app/routes/albums.py(1 hunks)backend/app/routes/face_clusters.py(3 hunks)backend/app/utils/images.py(2 hunks)docs/backend/backend_python/openapi.json(2 hunks)frontend/src/api/api-functions/face_clusters.ts(1 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/api/api-functions/face_clusters.ts (3)
frontend/src/types/API.ts (1)
APIResponse(1-8)frontend/src/api/axiosConfig.ts (1)
apiClient(5-12)frontend/src/api/apiEndpoints.ts (1)
faceClustersEndpoints(5-11)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (3)
frontend/src/features/loaderSlice.ts (2)
showLoader(17-20)hideLoader(21-24)frontend/src/api/api-functions/face_clusters.ts (1)
triggerGlobalReclustering(54-59)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)
backend/app/routes/face_clusters.py (6)
backend/app/utils/face_clusters.py (4)
cluster_util_cluster_all_face_embeddings(134-212)to_dict(41-48)_generate_cluster_face_image(513-549)_update_cluster_face_image(311-343)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(271-311)backend/app/database/metadata.py (2)
db_get_metadata(29-49)db_update_metadata(52-75)backend/app/utils/FaceNet.py (1)
FaceNet_util_cosine_similarity(20-23)backend/app/schemas/images.py (1)
ErrorResponse(53-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: Backend Tests
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
🔇 Additional comments (1)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (1)
33-75: Great end-to-end UX wiring for reclusteringLoader + dialog sequencing mirrors our existing update flow, so the new control feels native and keeps users informed through the long-running operation. Nicely done.
…sistency feat: enhance global face reclustering functionality with force option docs: update API description for global reclustering endpoint
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/utils/face_clusters.py (1)
103-123: Critical: Orphaned face cluster references after reclustering.After
db_delete_all_clusters()at line 103, any face marked as noise by DBSCAN (not present inresults) retains its oldcluster_id, which now points to a deleted cluster. Only faces present inresultsare updated bydb_update_face_cluster_ids_batch(results)at line 123, leaving orphaned faces with invalid foreign key references.Before inserting new clusters, clear
cluster_idfor all faces or only for faces not inresults:+from app.database.faces import db_get_all_faces_with_cluster_names + # Clear old clusters first db_delete_all_clusters() + + # Clear orphaned face cluster references + all_face_ids = { + face["face_id"] for face in db_get_all_faces_with_cluster_names() + } + reassigned_face_ids = {mapping["face_id"] for mapping in results} + faces_to_clear = [ + {"face_id": face_id, "cluster_id": None} + for face_id in all_face_ids - reassigned_face_ids + ] + if faces_to_clear: + db_update_face_cluster_ids_batch(faces_to_clear) # Extract unique clusters with their names (without face images yet)
🧹 Nitpick comments (4)
backend/app/database/folders.py (1)
385-387: Remove unnecessary parentheses around return type annotation.The extra parentheses wrapping the return type annotation serve no functional purpose in Python and are inconsistent with standard Python typing conventions.
Apply this diff:
-def db_get_all_folder_details() -> ( - List[Tuple[str, str, Optional[str], int, bool, Optional[bool]]] -): +def db_get_all_folder_details() -> List[Tuple[str, str, Optional[str], int, bool, Optional[bool]]]:backend/app/utils/face_clusters.py (1)
137-141: Consider clarifying return value semantics.The function returns different semantic values depending on the path taken:
- Full reclustering (line 137): cluster count
- Incremental (line 141): face assignment count
While this works for the current global reclustering endpoint (which always forces full reclustering), the inconsistency could confuse future callers or lead to incorrect result interpretation.
Consider one of these approaches:
Option 1: Return a structured result:
from typing import NamedTuple class ClusteringResult(NamedTuple): clusters_created: int faces_assigned: int was_full_recluster: bool # Update return statements accordinglyOption 2: Add type hints and document the behavior:
def cluster_util_face_clusters_sync(force_full_reclustering: bool = False) -> int: """ ... Returns: int: Number of clusters created (full reclustering) or number of faces assigned (incremental) """backend/app/routes/face_clusters.py (2)
325-325: Consider moving import to module level.Importing inside the function can impact performance on repeated calls and typically indicates a code organization issue. Unless this is necessary to avoid circular imports, move the import to the top of the file with other imports.
from app.schemas.images import AddSingleImageRequest from app.utils.FaceNet import FaceNet_util_cosine_similarity +from app.utils.face_clusters import cluster_util_face_clusters_syncThen remove line 325 from the function body.
336-340: Redundant ternary expression.Line 339 uses
result if result else 0, but since line 329 already handles theresult == 0case with an early return,resultis guaranteed to be non-zero at this point. The ternary is unnecessary.return GlobalReclusterResponse( success=True, message="Global reclustering completed successfully.", - clusters_created=result if result else 0, + clusters_created=result, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/database/faces.py(2 hunks)backend/app/database/folders.py(1 hunks)backend/app/routes/face_clusters.py(1 hunks)backend/app/utils/face_clusters.py(2 hunks)docs/backend/backend_python/openapi.json(2 hunks)frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/app/database/faces.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/backend/backend_python/openapi.json
- frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/routes/face_clusters.py (2)
backend/app/schemas/face_clusters.py (1)
ErrorResponse(23-26)backend/app/utils/face_clusters.py (1)
cluster_util_face_clusters_sync(84-141)
backend/app/utils/face_clusters.py (2)
backend/app/database/metadata.py (1)
db_get_metadata(29-49)backend/app/database/faces.py (1)
db_update_face_cluster_ids_batch(271-311)
🔇 Additional comments (4)
backend/app/utils/face_clusters.py (1)
84-91: LGTM! Clear parameter addition.The
force_full_reclusteringparameter is well-named, properly typed, and documented. TheFalsedefault preserves backward compatibility.backend/app/routes/face_clusters.py (3)
305-309: LGTM! Response model is well-defined.The
GlobalReclusterResponsemodel has clear, required fields that accurately represent the reclustering operation result.
311-320: LGTM! Endpoint definition is appropriate.The POST method, path, response model, and error handling configuration are all suitable for a manual reclustering trigger.
342-351: LGTM! Error handling is comprehensive.The exception handling properly logs errors and returns structured error responses. The exposure of exception details in the response is consistent with other endpoints in this file and acceptable for an internal admin operation.
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx
Outdated
Show resolved
Hide resolved
…ion for improved error handling and feedback
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (3)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/face_clusters.ts (1)
triggerGlobalReclustering(54-59)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
🔇 Additional comments (5)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (5)
2-7: LGTM!The Users icon import is properly added and used for the new "Recluster Faces" button.
18-20: LGTM!The required imports for the global reclustering feature are properly added. The use of
usePictoMutationanduseMutationFeedbackaddresses the previous review feedback. Based on learnings
40-42: LGTM!The mutation setup correctly uses
usePictoMutationwith the reclustering API function.
56-58: LGTM!The click handler correctly invokes the mutation with
undefinedsince the reclustering API requires no parameters.
117-117: LGTM!The UI updates are well-implemented:
- The description now appropriately mentions face clustering.
- The layout change to a responsive grid (
grid-cols-1 md:grid-cols-3) provides better responsive behavior.- The new "Recluster Faces" button follows the existing styling patterns, includes a helpful tooltip, and is properly wired to the handler.
Also applies to: 119-119, 142-152
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (1)
63-65: Consider disabling the button during reclustering.The handler correctly triggers the mutation. However, adding
disabled={reclusterMutation.isPending}to the button would prevent users from triggering multiple simultaneous reclustering operations.Apply this diff to disable the button while reclustering is in progress:
<Button onClick={onGlobalReclusterClick} + disabled={reclusterMutation.isPending} variant="outline" className="flex h-12 w-full gap-3" title="Rebuild all face clusters from scratch using latest clustering algorithms" >Also applies to: 149-159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (3)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/face_clusters.ts (1)
triggerGlobalReclustering(54-59)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
🔇 Additional comments (6)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (6)
2-7: LGTM! Icon import added correctly.The
Usersicon import from lucide-react is appropriate for the face reclustering button.
18-20: Previous review feedback addressed correctly.The imports for
triggerGlobalReclustering,usePictoMutation, anduseMutationFeedbackcorrectly implement the pattern suggested in prior review comments.
40-43: Mutation setup is correct.The
usePictoMutationconfiguration properly wires the reclustering API and ensures cluster queries are invalidated after completion.
45-60: Correctly implements reactive feedback options.This implementation matches Option 2 from the previous review comment. The
useMemodependencies onreclusterMutation.successMessageandreclusterMutation.errorMessageensure thatuseMutationFeedbackreceives the updated backend messages when the mutation completes, resolving the timing issue flagged earlier.
124-124: Description accurately reflects the expanded functionality.
126-126: Responsive grid layout improves button presentation.The switch to a responsive grid (
grid-cols-1 md:grid-cols-3) ensures consistent sizing and spacing for the three control buttons across different screen sizes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (2)
60-64: Consider adding user confirmation before reclustering.While the mutation and feedback hooks are correctly implemented, global face reclustering can be a resource-intensive operation. Consider adding a confirmation dialog before starting the operation to prevent accidental triggers.
Example implementation:
const onGlobalReclusterClick = () => { + dispatch( + showInfoDialog({ + title: 'Confirm Global Reclustering', + message: 'This will rebuild all face clusters from scratch. This operation may take several minutes. Do you want to continue?', + variant: 'warning', + onConfirm: () => { reclusterMutation.mutate(undefined); + }, + }), + ); - reclusterMutation.mutate(undefined); };Note: Verify that
showInfoDialogsupportsonConfirmcallback, or use an alternative confirmation dialog component.
148-158: LGTM! Button implementation is consistent and well-structured.The new button follows the established patterns and includes a helpful tooltip description.
Optional: Disable button during operation.
While the loading feedback provides visual indication, explicitly disabling the button prevents accidental multiple clicks:
<Button onClick={onGlobalReclusterClick} variant="outline" className="flex h-12 w-full gap-3" title="Rebuild all face clusters from scratch using latest clustering algorithms" + disabled={reclusterMutation.isPending} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (3)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/face_clusters.ts (1)
triggerGlobalReclustering(54-59)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tauri Build Check (windows-latest)
🔇 Additional comments (5)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (5)
2-7: LGTM! Imports are correct and properly used.The new imports for the Users icon, API function, and mutation hooks are all properly utilized in the component.
Also applies to: 18-20
40-43: LGTM! Mutation setup follows best practices.The mutation is correctly configured with automatic cache invalidation for cluster-related queries.
45-58: LGTM! Dynamic message handling is correctly implemented.The memoized feedback options with proper dependencies ensure that backend messages are displayed when available while providing sensible fallbacks. This correctly addresses the previous review concern about non-functional dynamic message references.
123-123: LGTM! Description accurately reflects new functionality.The updated description clearly communicates that face clustering management is now available in this section.
125-125: LGTM! Responsive grid layout accommodates the new button well.The grid layout provides good structure for the three control buttons with appropriate responsive behavior.
|
@Hemil36 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/utils/face_clusters.py (1)
106-139: Transaction safety not implemented despite docstring claim.The docstring promises "transaction safety" but the code performs multiple separate database operations without wrapping them in a transaction. If any operation fails (delete, insert, update faces, update metadata), the database will be left in an inconsistent state.
For a user-triggered global recluster operation, this is a critical data integrity risk.
Wrap the full reclustering block in a transaction:
import sqlite3 from app.config.settings import DATABASE_PATH conn = sqlite3.connect(DATABASE_PATH) try: conn.execute("BEGIN TRANSACTION") # Perform clustering operation results = cluster_util_cluster_all_face_embeddings() if not results: conn.rollback() return 0 results = [result.to_dict() for result in results] # All database operations within the transaction # ... (delete, insert, update operations) conn.commit() return len(cluster_list) except Exception as e: conn.rollback() logger.error(f"Error during face clustering: {e}") raise finally: conn.close()Alternatively, refactor the database functions to accept a connection object and manage the transaction at this level.
🧹 Nitpick comments (3)
backend/app/utils/face_clusters.py (2)
101-102: Verify: Silent success on empty clustering results.Returning
0whenresultsis empty may be ambiguous—it could mean either "no faces to cluster" or "clustering produced no valid clusters." Consider logging an info message here to distinguish this case from the normal success path at line 141.Add logging to clarify the early return:
if not results: + logger.info("No clustering results to process (no faces or all noise)") return 0
141-145: Note: Return values have different semantics.Line 141 returns the number of clusters created (full reclustering), while line 145 returns the number of face assignments made (incremental). Both are valid success metrics, but callers should be aware that the meaning differs based on which path is taken.
Consider documenting the return value semantics in the docstring:
def cluster_util_face_clusters_sync(force_full_reclustering: bool = False): """ Smart face clustering with transaction safety. Decides between full reclustering or incremental assignment based on 24-hour rule and face count. Args: force_full_reclustering: If True, forces full reclustering regardless of 24-hour rule + + Returns: + int: Number of clusters created (full reclustering) or number of faces assigned (incremental) """frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (1)
148-158: Minor styling inconsistency with other buttons.The new "Recluster Faces" button is missing the
cursor-pointerclass that's present on the other two buttons (lines 129 and 140). While buttons have a pointer cursor by default, maintaining consistency with the existing buttons would be cleaner.Apply this diff for consistency:
<Button onClick={onGlobalReclusterClick} variant="outline" - className="flex h-12 w-full gap-3" + className="flex h-12 w-full cursor-pointer gap-3" title="Rebuild all face clusters from scratch using latest clustering algorithms" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/routes/face_clusters.py(1 hunks)backend/app/utils/face_clusters.py(2 hunks)docs/backend/backend_python/openapi.json(2 hunks)frontend/src/api/api-functions/face_clusters.ts(1 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/api/apiEndpoints.ts
- docs/backend/backend_python/openapi.json
- backend/app/routes/face_clusters.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/app/utils/face_clusters.py (2)
backend/app/database/metadata.py (1)
db_get_metadata(33-55)backend/app/database/faces.py (1)
db_update_face_cluster_ids_batch(288-330)
frontend/src/api/api-functions/face_clusters.ts (3)
frontend/src/types/API.ts (1)
APIResponse(1-8)frontend/src/api/axiosConfig.ts (1)
apiClient(5-12)frontend/src/api/apiEndpoints.ts (1)
faceClustersEndpoints(5-12)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (3)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/face_clusters.ts (1)
triggerGlobalReclustering(68-73)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
🔇 Additional comments (10)
backend/app/utils/face_clusters.py (2)
88-95: LGTM! Backward-compatible API enhancement.The new
force_full_reclusteringparameter is well-documented and maintains backward compatibility with existing callers.
97-97: LGTM! Correctly implements forced reclustering.The conditional logic properly bypasses the 24-hour rule when
force_full_reclusteringis True, as intended by the PR objectives.frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (7)
2-7: LGTM!Icon imports are properly organized and the new
Usersicon is correctly used for the face reclustering button.
18-20: LGTM!Required imports for the mutation flow are properly added and used throughout the component.
40-43: LGTM!The mutation is properly configured with automatic cache invalidation for the 'clusters' tag, ensuring the UI reflects the new clustering results.
45-58: LGTM!The feedback options correctly use reactive properties from
usePictoMutationwith proper memoization, addressing the previous review comment. The implementation now properly handles dynamic messages from the backend.
60-64: LGTM!The mutation feedback hook is correctly applied, and the click handler properly triggers the mutation without parameters.
123-123: LGTM!The card description is appropriately updated to reflect the new face clustering functionality.
125-125: LGTM!The responsive grid layout properly accommodates the three action buttons with appropriate breakpoints.
frontend/src/api/api-functions/face_clusters.ts (1)
68-73: Verify API timeout for long-running operation.The
apiClienthas a 10-second timeout. The/face-clusters/global-reclusterendpoint performs a synchronous operation that returns full results immediately (includingclusters_createddata in the response). For large photo collections, this global reclustering operation may exceed the 10-second timeout. Verify whether this timeout is adequate for your typical use cases or consider extending it for this endpoint.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/routes/face_clusters.py(2 hunks)backend/app/schemas/face_clusters.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/routes/face_clusters.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). (4)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
…a models; update related components for improved response handling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/routes/face_clusters.py (1)
304-346: Consider moving the import to module level.The import of
cluster_util_face_clusters_syncon Line 318 is placed inside the function. While this works, imports are conventionally placed at the module level in Python. Moving it to the top improves IDE support, linting accuracy, and marginally improves performance.Apply this diff to move the import to the module level:
from app.database.face_clusters import ( db_get_cluster_by_id, db_update_cluster, db_get_all_clusters_with_face_counts, db_get_images_by_cluster_id, # Add this import ) +from app.utils.face_clusters import cluster_util_face_clusters_sync from app.schemas.face_clusters import (Then remove the import from inside the function:
try: logger.info("Starting manual global face reclustering...") - # Use the smart clustering function with force flag set to True - from app.utils.face_clusters import cluster_util_face_clusters_sync - result = cluster_util_face_clusters_sync(force_full_reclustering=True)Note: If this import is placed inside the function to avoid a circular import, you may safely disregard this suggestion and add a comment explaining the reason.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/routes/face_clusters.py(2 hunks)backend/app/schemas/face_clusters.py(1 hunks)docs/backend/backend_python/openapi.json(2 hunks)frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/routes/face_clusters.py (2)
backend/app/schemas/face_clusters.py (3)
GlobalReclusterResponse(81-85)GlobalReclusterData(77-78)ErrorResponse(23-26)backend/app/utils/face_clusters.py (1)
cluster_util_face_clusters_sync(88-145)
frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (3)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/face_clusters.ts (1)
triggerGlobalReclustering(68-73)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)
🔇 Additional comments (8)
backend/app/schemas/face_clusters.py (1)
77-85: LGTM! Previous feedback addressed.The response model structure now correctly aligns with the established pattern used by other response models in this file. The use of optional fields for
message,error, and nesteddataprovides consistent error handling across the API.docs/backend/backend_python/openapi.json (2)
1145-1176: LGTM! OpenAPI spec correctly documents the new endpoint.The endpoint definition is clear and consistent with other Face Clusters endpoints. The description accurately conveys that this operation bypasses the normal 24-hour reclustering rule.
1997-2058: LGTM! Schema definitions align with backend models.The OpenAPI schema definitions for
GlobalReclusterDataandGlobalReclusterResponsecorrectly match the Pydantic models and follow the established response pattern throughout the API.backend/app/routes/face_clusters.py (1)
21-22: LGTM! Imports follow the established pattern.The new response model imports are correctly placed with other schema imports from
app.schemas.face_clusters.frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx (4)
2-7: LGTM! Icon import updated correctly.The
Usersicon fromlucide-reactis appropriately chosen for the face reclustering feature.
18-20: LGTM! Required dependencies imported.The imports for
triggerGlobalReclustering,usePictoMutation, anduseMutationFeedbackare correctly added to support the new reclustering feature.
40-70: LGTM! Mutation and feedback properly configured.The implementation correctly addresses previous review feedback:
- Uses
usePictoMutationwith proper configuration- Memoizes feedback options with correct dependencies to ensure dynamic messages update reactively
- Auto-invalidates the
clustersquery key to refresh cluster data after reclustering- Properly extracts
successMessageanderrorMessagefrom the mutation state
129-165: LGTM! UI updated with new reclustering control.The changes are well-executed:
- Updated description accurately reflects the new capability
- Grid layout provides a clean, responsive design for the three controls
- The "Recluster Faces" button includes a helpful
titleattribute explaining its purpose- Button styling is consistent with the existing controls
rahulharpal1603
left a comment
rahulharpal1603
left a comment
There was a problem hiding this comment.
Thanks @Hemil36!
2 participants
Fixes : #550
This pull request introduces a new feature that allows users to manually trigger a global face reclustering operation from the application's settings page. This operation rebuilds all face clusters from scratch, bypassing the usual 24-hour check, and updates both the backend API and frontend UI to support this functionality. Additionally, several minor code cleanups and improvements are included.
Global Face Reclustering Feature:
/face-clusters/global-recluster(POST) that performs a full reclustering of all faces, updates clusters and assignments, generates cluster images, and updates metadata. Returns a response with the number of clusters created. (backend/app/routes/face_clusters.py,docs/backend/backend_python/openapi.json)GlobalReclusterResponsefor the API, and documented the endpoint in OpenAPI. (docs/backend/backend_python/openapi.json)backend/app/routes/face_clusters.py,backend/app/database/face_clusters.py)Frontend Integration:
triggerGlobalReclusteringand endpoint mapping for the global recluster operation. (frontend/src/api/api-functions/face_clusters.ts,frontend/src/api/apiEndpoints.ts)ApplicationControlsCard) to include a new "Recluster Faces" button, which calls the new API, shows loader/info dialogs, and handles errors. Updated the UI layout and description accordingly. (frontend/src/pages/SettingsPage/components/ApplicationControlsCard.tsx)Summary by CodeRabbit
New Features
Documentation