Fix: Favorite Icon Updates Correctly on PersonImages Page #681 #682
Conversation
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant U as User (UI)
participant C as React Query Client
participant H as useToggleFav Hook
participant S as Backend API
participant DB as Database
participant R as Redux Store
U->>H: toggleFavourite(imageId)
H->>C: onMutate -> cancel queries, snapshot previousImages
H->>C: update cache optimistically (set isFavourite)
H->>S: PATCH /images/{id}/favourite
S->>DB: update is_favourite
DB-->>S: result
S-->>H: 200 OK
H->>C: onSuccess -> invalidate ['person-images', clusterId]
C-->>C: refetch person-images query
C->>S: GET /clusters/{id}/images
S->>DB: select images (includes isFavourite)
DB-->>S: images
S-->>C: response with images
C->>R: cache subscription fires -> dispatch setImages(updatedImages)
alt error
S-->>H: 4xx/5xx
H->>C: onError -> rollback snapshot, invalidate person-images
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
🧹 Nitpick comments (4)
backend/app/schemas/face_clusters.py (1)
54-54: Remove leftover development comment.The inline comment
# Add this fieldappears to be a development note that should be removed before merging.- isFavourite: bool = False # Add this field + isFavourite: bool = Falsefrontend/src/hooks/useToggleFav.ts (1)
16-62: Incomplete optimistic update rollback forperson-images.The
onMutateoptimistically updates bothimagesandperson-imagescaches, but on error, onlypreviousImages(theimagescache snapshot) is restored. Theperson-imagescache isn't explicitly rolled back. While the subsequentinvalidateQuerieson line 61 will refetch and correct the state, this creates a brief window of inconsistency.For a more robust pattern, consider snapshotting and restoring both caches:
onMutate: async (image_id: string) => { // Cancel outgoing refetches await queryClient.cancelQueries({ queryKey: ['images'] }); + await queryClient.cancelQueries({ queryKey: ['person-images'] }); // Snapshot the previous value const previousImages = queryClient.getQueryData(['images']); + const previousPersonImages = queryClient.getQueriesData({ queryKey: ['person-images'] }); // Optimistically update images query // ... existing code ... - return { previousImages }; + return { previousImages, previousPersonImages }; }, onError: (err, image_id, context) => { if (context?.previousImages) { queryClient.setQueryData(['images'], context.previousImages); } + if (context?.previousPersonImages) { + context.previousPersonImages.forEach(([queryKey, data]: [unknown[], unknown]) => { + queryClient.setQueryData(queryKey, data); + }); + } // Refetch to restore correct state queryClient.invalidateQueries({ queryKey: ['person-images'] }); },frontend/src/pages/PersonImages/PersonImages.tsx (1)
52-68: Cache subscription approach works, but consider simplification.The cache subscription correctly syncs optimistic updates from
useToggleFavto Redux. However, this creates dual state management: the existinguseEffect(lines 38-50) handles query state transitions, while this new subscription handles cache mutations.A simpler alternative would be to have
useToggleFavreturn the updated images directly or use React Query's state as the source of truth instead of syncing to Redux. That said, if Redux is needed elsewhere, this approach is acceptable.backend/app/routes/face_clusters.py (1)
172-183: Consider using a list comprehension for cleaner transformation.The manual loop building
formatted_imagesis verbose. A list comprehension would be more Pythonic and concise.Apply this diff to refactor using a list comprehension:
- # Transform the data to match the frontend schema - formatted_images = [] - for img in images: - formatted_images.append({ - "id": img["image_id"], # Changed from image_id to id - "path": img["image_path"], # Changed from image_path to path - "thumbnailPath": img["thumbnail_path"], # Changed from thumbnail_path to thumbnailPath - "metadata": img["metadata"], - "isFavourite": bool(img["isFavourite"]), # Ensure boolean - "face_id": img["face_id"], - "confidence": img["confidence"], - "bbox": img["bbox"], - }) + # Transform the data to match the frontend schema + formatted_images = [ + { + "id": img["image_id"], + "path": img["image_path"], + "thumbnailPath": img["thumbnail_path"], + "metadata": img["metadata"], + "isFavourite": bool(img["isFavourite"]), + "face_id": img["face_id"], + "confidence": img["confidence"], + "bbox": img["bbox"], + } + for img in images + ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/database/face_clusters.py(3 hunks)backend/app/routes/face_clusters.py(1 hunks)backend/app/schemas/face_clusters.py(1 hunks)docs/backend/backend_python/openapi.json(1 hunks)frontend/src/hooks/useToggleFav.ts(1 hunks)frontend/src/pages/PersonImages/PersonImages.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/routes/face_clusters.py (2)
backend/app/database/face_clusters.py (2)
db_get_cluster_by_id(126-153)db_get_images_by_cluster_id(281-354)backend/app/schemas/face_clusters.py (3)
ErrorResponse(23-26)GetClusterImagesResponse(69-75)GetClusterImagesData(60-66)
🔇 Additional comments (4)
backend/app/database/face_clusters.py (1)
304-345: LGTM!The
isFavouritefield is correctly added to the SQL query, tuple unpacking, and output dictionary. The naming convention is consistent with the existing codebase patterns.docs/backend/backend_python/openapi.json (1)
2215-2219: LGTM!The OpenAPI spec correctly documents the new
isFavouritefield with appropriate type, title, and default value, consistent with the backend Pydantic model.frontend/src/pages/PersonImages/PersonImages.tsx (1)
30-30: EnablingrefetchOnWindowFocusis appropriate for this use case.This ensures the favorite state stays in sync when users switch between tabs, which aligns with the PR objective of keeping the UI consistent.
backend/app/routes/face_clusters.py (1)
196-207: LGTM: Improved error handling and logging.The addition of
logger.errorfor debugging and the updated error messages improve observability and clarity. The exception handling pattern correctly re-raisesHTTPExceptionwhile catching and logging generic exceptions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/routes/face_clusters.py (1)
187-192: Critical:clusteris aClusterDataobject, not a dict – fixcluster_nameaccess
db_get_cluster_by_idreturns aClusterDataobject (seebackend/app/database/face_clusters.py), socluster["cluster_name"]will raise aTypeErrorat runtime becauseClusterDatais not subscriptable. This will break the/clusters/{cluster_id}/imagesendpoint whenever a valid cluster exists.Use attribute access instead, and consider removing the stale inline comment:
- data=GetClusterImagesData( - cluster_id=cluster_id, - cluster_name=cluster["cluster_name"], # ✅ CHANGE THIS LINE - Remove .get() - images=formatted_images, - total_images=len(formatted_images), - ), + data=GetClusterImagesData( + cluster_id=cluster_id, + cluster_name=cluster.cluster_name, + images=formatted_images, + total_images=len(formatted_images), + ),This keeps the response in sync with the
ClusterDatadefinition and avoids a hard runtime failure.
🧹 Nitpick comments (2)
backend/app/routes/face_clusters.py (2)
168-183: Image transformation from DB rows to API payload looks correctThe mapping from
db_get_images_by_cluster_idrows toformatted_imagesfields (id,path,thumbnailPath,metadata,isFavourite,face_id,confidence,bbox) is consistent with the database snippet provided and normalizingisFavouriteviabool(...)is appropriate for the API.If you want to simplify later, this block could be expressed as a list comprehension for slightly tighter code, but the current explicit loop is perfectly fine and readable.
199-205: Logging on failure is good; consider not returning raw exception textAdding
logger.error(...)on the generic exception path is useful for observability, and the updated error text is fine. However, you’re still includingstr(e)in the client-facingmessage, which can leak internal details. A safer pattern is usually:
- Log
str(e)server-side.- Return a stable, generic message to the client.
Not blocking this PR, but worth considering for future hardening.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/routes/face_clusters.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/routes/face_clusters.py (2)
backend/app/database/face_clusters.py (2)
db_get_cluster_by_id(126-153)db_get_images_by_cluster_id(281-354)backend/app/schemas/face_clusters.py (1)
GetClusterImagesData(60-66)
🔇 Additional comments (1)
backend/app/routes/face_clusters.py (1)
156-166: 404 handling for missing clusters is consistent and clearThe existence check and 404 response for a missing
cluster_idremain correct, and the updated message wording is fine and consistent with the rest of the file.
1 participant
Fixes #681
This PR fixes the issue where the favorite (heart) icon on the PersonImages page does not update immediately after toggling favorites.
Issue: #681 – Favorite Icon Does Not Update on PersonImages Page
Changes:
Steps to verify:
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.