Adding functionality for add and remove from favourite#551
Conversation
WalkthroughAdds an isFavourite flag across DB, API, OpenAPI and frontend; implements a backend POST /images/toggle-favourite endpoint and db toggle function; adds frontend API, hook, UI wiring, a Favourites page, sidebar route, and routing constant. Changes
Sequence Diagram(s)%%{init: {"themeVariables":{"primaryColor":"#E6F0FF","secondaryColor":"#FFF0F6","actorBorder":"#2B6CB0"}}}%%
sequenceDiagram
participant UI as ImageCard / MediaView
participant Handler as Local Handler
participant Hook as useToggleFav
participant API as apiClient
participant Backend as Backend API
participant DB as images DB
UI->>Handler: user clicks favourite
Handler->>Hook: toggleFavourite(image_id)
Hook->>API: POST /images/toggle-favourite { image_id }
API->>Backend: HTTP request
Backend->>DB: db_toggle_image_favourite_status toggles 0↔1
DB-->>Backend: success / error
Backend-->>API: 200 OK / 4xx
API-->>Hook: response
Hook->>Hook: invalidate 'images' cache tag
Hook-->>Handler: result
Handler->>UI: update local isfav / re-render
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
frontend/src/pages/Home/MyFav.tsx (2)
72-86: Consider using the Heart icon component for consistency.The empty state uses an inline SVG for the heart icon. For consistency with the rest of the UI (which uses Lucide icons), consider using the
Hearticon fromlucide-react.Apply this diff:
+import { Heart } from 'lucide-react'; {/* Heart Icon/Sticker */} <div className="mb-6 flex h-32 w-32 items-center justify-center rounded-full bg-muted/50"> - <svg - className="h-16 w-16 text-muted-foreground/60" - fill="none" - stroke="currentColor" - viewBox="0 0 24 24" - > - <path - strokeLinecap="round" - strokeLinejoin="round" - strokeWidth={1.5} - d="M4.318 6.318a4.5 4.5 0 000 6.364L12 20.364l7.682-7.682a4.5 4.5 0 00-6.364-6.364L12 7.636l-1.318-1.318a4.5 4.5 0 00-6.364 0z" - /> - </svg> + <Heart className="h-16 w-16 text-muted-foreground/60" /> </div>
98-103: Consider using react-router navigation and Button component.Two minor suggestions:
window.history.back()is not ideal as it relies on browser history. Consider usingnavigate(ROUTES.HOME)fromreact-routerfor more predictable behavior.- The button uses inline classes. Consider using the
Buttoncomponent from your UI library for consistency with the rest of the application.Apply this diff:
+import { useNavigate } from 'react-router'; +import { Button } from '@/components/ui/button'; +import { ROUTES } from '@/constants/routes'; + const navigate = useNavigate(); {/* Optional: Browse Images Button */} - <button - onClick={() => window.history.back()} - className="rounded-lg bg-primary px-6 py-2 cursor-pointer text-primary-foreground transition-colors hover:bg-primary/90" - > + <Button onClick={() => navigate(`/${ROUTES.HOME}`)}> Browse Images - </button> + </Button>backend/app/database/images.py (1)
351-382: Consider extracting schema migration and remove commented-out code.The function mixes runtime schema migration (lines 359-364) with data access logic. While this provides a fallback for existing databases, it's not ideal to perform schema changes in a data access function.
Refactor suggestion:
Remove the runtime schema migration logic since
db_create_images_tablealready includes theisFavouritecolumn (line 45). For existing databases, handle schema updates through a proper migration script.Remove commented-out print statements (lines 373, 375, 378).
For consistency with other database functions, consider removing or logging the print statements (lines 357, 362) instead of printing directly to stdout.
Example refactored version:
def db_toggle_image_favourite_status(image_id: str) -> bool: """Toggle the favourite status of an image.""" conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() try: # Check if image exists cursor.execute("SELECT id FROM images WHERE id = ?", (image_id,)) if not cursor.fetchone(): return False # Toggle favourite status cursor.execute( """ UPDATE images SET isFavourite = CASE WHEN isFavourite = 1 THEN 0 ELSE 1 END WHERE id = ? """, (image_id,), ) conn.commit() return cursor.rowcount > 0 except Exception as e: conn.rollback() return False finally: conn.close()If you need to support existing databases without the
isFavouritecolumn, consider creating a dedicated migration script that runs once during application startup or deployment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
__pycache__/app.cpython-313.pycis excluded by!**/*.pycutils/__pycache__/cache.cpython-313.pycis excluded by!**/*.pyc
📒 Files selected for processing (14)
backend/app/database/images.py(5 hunks)backend/app/routes/images.py(4 hunks)backend/run.bat(0 hunks)docs/backend/backend_python/openapi.json(5 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/components/Media/ImageCard.tsx(3 hunks)frontend/src/components/Media/MediaView.tsx(6 hunks)frontend/src/components/Navigation/Sidebar/AppSidebar.tsx(2 hunks)frontend/src/constants/routes.ts(1 hunks)frontend/src/hooks/useFavorites.ts(0 hunks)frontend/src/pages/Home/Home.tsx(0 hunks)frontend/src/pages/Home/MyFav.tsx(1 hunks)frontend/src/routes/AppRoutes.tsx(2 hunks)frontend/src/types/Media.ts(1 hunks)
💤 Files with no reviewable changes (3)
- frontend/src/hooks/useFavorites.ts
- backend/run.bat
- frontend/src/pages/Home/Home.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/routes/AppRoutes.tsx (2)
frontend/src/constants/routes.ts (1)
ROUTES(1-12)frontend/src/pages/Home/MyFav.tsx (1)
MyFav(14-131)
frontend/src/components/Navigation/Sidebar/AppSidebar.tsx (1)
frontend/src/constants/routes.ts (1)
ROUTES(1-12)
frontend/src/pages/Home/MyFav.tsx (10)
frontend/src/features/imageSelectors.ts (2)
selectIsImageViewOpen(24-27)selectImages(5-7)frontend/src/app/store.ts (1)
RootState(22-22)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/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)frontend/src/types/Media.ts (1)
Image(1-11)frontend/src/features/imageSlice.ts (1)
setImages(22-26)frontend/src/components/Media/ImageCard.tsx (1)
ImageCard(20-120)frontend/src/components/Media/MediaView.tsx (1)
MediaView(27-218)
backend/app/routes/images.py (2)
backend/app/database/images.py (2)
db_toggle_image_favourite_status(351-382)db_get_all_images(94-169)backend/app/routes/face_clusters.py (1)
ImageData(40-48)
🔇 Additional comments (18)
frontend/src/routes/AppRoutes.tsx (2)
8-8: LGTM!The import statement for
MyFavfollows the existing pattern and is correctly placed.
20-20: LGTM!The new Favourites route is properly integrated within the Layout and follows the existing routing pattern.
frontend/src/components/Navigation/Sidebar/AppSidebar.tsx (2)
15-15: LGTM!The Heart icon import is correctly placed and follows the existing pattern for other Lucide icons.
41-41: LGTM!The Favourites menu item is properly configured and follows the existing pattern for sidebar navigation items.
frontend/src/constants/routes.ts (1)
3-3: LGTM!The FAVOURITES route constant is correctly added and follows the existing naming convention.
frontend/src/types/Media.ts (1)
8-8: LGTM!The
isFavouriteproperty is correctly added as an optional boolean field, which is appropriate for backward compatibility and aligns with the new favourites feature.frontend/src/api/apiEndpoints.ts (1)
3-3: LGTM!The
setfavouriteendpoint is correctly added and follows the existing pattern for API endpoint definitions.frontend/src/pages/Home/MyFav.tsx (3)
52-54: Empty handler is appropriate here.The empty
handleCloseMediaViewfunction with explanatory comment is fine. TheMediaViewcomponent handles closing via Redux actions, so no additional logic is needed here.
24-50: Query and lifecycle management looks good.The use of
usePictoQuerywith proper enabled flag, loading states, error handling, and Redux integration is well-implemented and follows best practices.
56-64: Filtering and rendering logic is correct.The logic to filter favourite images and conditionally render either the empty state or the image grid is well-structured and handles all cases appropriately. The use of
favouriteImagesfor both the grid andMediaViewensures consistent navigation.Also applies to: 109-130
backend/app/routes/images.py (3)
6-6: LGTM!The import of
db_toggle_image_favourite_statusis correctly placed and matches the function definition inbackend/app/database/images.py.
11-19: LGTM!The addition of
isFavourite: boolto theImageDatamodel is consistent with the database schema changes and correctly marks it as a required field.
40-52: LGTM!The use of
image.get("isFavourite", False)provides a safe fallback for cases where the field might not be present, ensuring backward compatibility. The implementation correctly propagates theisFavouritefield to the response.docs/backend/backend_python/openapi.json (3)
899-940: LGTM!The
GET /images/info/{image_id}endpoint is correctly defined with proper path parameter and response schema.
1996-2052: LGTM!The addition of
isFavouriteto theImageDataschema is consistent with the backend model changes and correctly marks it as a required boolean field.
2147-2203: LGTM!The new schemas
ImageInfoResponseandToggleFavouriteRequestare correctly defined and match the corresponding backend models.Also applies to: 2405-2417
backend/app/database/images.py (2)
31-49: LGTM!The addition of
isFavourite BOOLEAN DEFAULT 0to the images table schema is correctly implemented and follows the existing pattern for boolean columns.
105-169: LGTM!The modifications to
db_get_all_imagescorrectly retrieve and propagate theisFavouritefield, with appropriate boolean conversion.
| }: ImageCardProps) { | ||
| const dispatch = useDispatch(); | ||
| const [isFavorite, setIsFavorite] = useState(false); | ||
| const [isFavorite, setIsFavorite] = useState(image.isFavourite); |
There was a problem hiding this comment.
Local state out of sync with props and no backend persistence.
Multiple critical issues:
isFavoritelocal state is initialized fromimage.isFavouritebut never re-syncs whenimage.isFavouritechanges externally (e.g., when another component updates the same image or on re-fetch).handle_favourite_toggleonly updates local state—it doesn't call the backend API to persist the change.- The UI logic relies on
image.isFavourite(lines 80, 86) while the toggle handler updatesisFavorite(local state), creating an inconsistency.
This means clicking the favorite button will not persist the change to the backend, and the UI state can become stale or inconsistent with the server.
Solution: Remove the local isFavorite state and rely solely on image.isFavourite from props. Implement handle_favourite_toggle to call the backend API (using the setfavourite endpoint from apiEndpoints.ts) and dispatch a Redux action to update the image in the global store.
Example pattern (adjust to your Redux setup):
- const [isFavorite, setIsFavorite] = useState(image.isFavourite);
+ // Remove local state; use image.isFavourite from props
- const handle_favourite_toggle = () => {
- setIsFavorite(!isFavorite);
- };
+ const handle_favourite_toggle = async (e: React.MouseEvent) => {
+ e.stopPropagation(); // Prevent card click
+ try {
+ const response = await axios.post(
+ `${API_BASE_URL}${imagesEndpoints.setfavourite}`,
+ { image_id: image.id }
+ );
+ if (response.data.success) {
+ // Dispatch Redux action to update image.isFavourite in store
+ dispatch(updateImageFavourite({
+ id: image.id,
+ isFavourite: response.data.isFavourite
+ }));
+ }
+ } catch (error) {
+ console.error('Failed to toggle favourite:', error);
+ // Show error notification
+ }
+ };Additionally, update line 84 to prevent event propagation:
- onClick={handle_favourite_toggle}
+ onClick={(e) => { e.stopPropagation(); handle_favourite_toggle(e); }}Also applies to: 34-36
| className={`cursor-pointer rounded-full p-2.5 text-white transition-all duration-300 ${ | ||
| image.isFavourite | ||
| ? 'bg-rose-500/80 hover:bg-rose-600 hover:shadow-lg' | ||
| : 'bg-white/10 hover:bg-white/20 hover:shadow-lg' | ||
| }`} | ||
| onClick={handle_favourite_toggle} | ||
| > | ||
| <Heart | ||
| className={cn( | ||
| 'h-5 w-5', | ||
| isFavorite ? 'fill-brand-orange text-brand-orange' : '', | ||
| )} | ||
| /> | ||
| {image.isFavourite ? ( | ||
| // Filled Heart (when favourite) | ||
| <Heart | ||
| className="h-5 w-5" | ||
| fill="currentColor" | ||
| > | ||
| </Heart> | ||
| ) : ( | ||
| <Heart className="h-5 w-5" /> | ||
| )} |
There was a problem hiding this comment.
Inconsistent state usage in UI logic.
The button styling (lines 79-83) and icon rendering (lines 86-95) read image.isFavourite from props, but the onClick handler (line 84) calls handle_favourite_toggle which toggles the local isFavorite state. This creates a disconnect where the UI renders one state but the handler modifies another.
This is a consequence of the critical issue flagged above. Once you remove the local state and implement proper backend synchronization, this inconsistency will be resolved.
| const { viewState, handlers } = useImageViewControls(); | ||
| const { favorites, toggleFavorite, isFavorite } = useFavorites(); | ||
|
|
||
| const [isfav, setIsfav] = useState(currentImage?.isFavourite || false); |
There was a problem hiding this comment.
Stale local state when navigating images.
isfav is initialized once from currentImage?.isFavourite but doesn't update when currentImage changes (e.g., when the user navigates to the next/previous image). This means the favorite icon state will be incorrect after navigation.
Add a useEffect to sync isfav with currentImage?.isFavourite:
const [isfav, setIsfav] = useState(currentImage?.isFavourite || false);
+
+ useEffect(() => {
+ setIsfav(currentImage?.isFavourite || false);
+ }, [currentImage?.isFavourite]);📝 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 [isfav, setIsfav] = useState(currentImage?.isFavourite || false); | |
| const [isfav, setIsfav] = useState(currentImage?.isFavourite || false); | |
| useEffect(() => { | |
| setIsfav(currentImage?.isFavourite || false); | |
| }, [currentImage?.isFavourite]); |
🤖 Prompt for AI Agents
In frontend/src/components/Media/MediaView.tsx around line 47, the local state
variable isfav is only initialized from currentImage?.isFavourite and does not
update when currentImage changes, causing the favorite icon to become stale when
navigating images; add a useEffect that depends on currentImage (or
currentImage?.isFavourite) and calls setIsfav(currentImage?.isFavourite ||
false) to synchronize the local state whenever the displayed image changes,
ensuring the UI reflects the current image's favorite status.
|
@rahulharpal1603 please review sir |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/backend/backend_python/openapi.json (1)
2146-2283: Resolve extensive merge conflicts in schema definitions.Unresolved merge conflict markers are breaking the JSON structure and causing numerous parse errors (reported by Biome static analysis).
The conflict spans the
ImageInfoResponseandMetadataModelschema definitions. Both schemas should be preserved:
- Keep
ImageInfoResponse(lines 2147-2272 from HEAD) - needed for the new/images/info/{image_id}endpoint- Keep
MetadataModel(lines 2185-2283 from 589d71f) - needed for structured metadata inImageDataHowever, align
ImageInfoResponse.metadatato referenceMetadataModelinstead of using type"string":"ImageInfoResponse": { "properties": { "id": {"type": "string", "title": "Id"}, "path": {"type": "string", "title": "Path"}, "folder_id": {"type": "string", "title": "Folder Id"}, "thumbnailPath": {"type": "string", "title": "Thumbnailpath"}, "metadata": { "$ref": "#/components/schemas/MetadataModel" }, "isTagged": {"type": "boolean", "title": "Istagged"}, "isFavourite": {"type": "boolean", "title": "Isfavourite"}, "tags": { "anyOf": [ {"items": {"type": "string"}, "type": "array"}, {"type": "null"} ], "title": "Tags" } }, "type": "object", "required": ["id", "path", "folder_id", "thumbnailPath", "metadata", "isTagged", "isFavourite"], "title": "ImageInfoResponse" }, "MetadataModel": { "properties": { "name": {"type": "string", "title": "Name"}, "date_created": { "anyOf": [{"type": "string"}, {"type": "null"}], "title": "Date Created" }, "width": {"type": "integer", "title": "Width"}, "height": {"type": "integer", "title": "Height"}, "file_location": {"type": "string", "title": "File Location"}, "file_size": {"type": "integer", "title": "File Size"}, "item_type": {"type": "string", "title": "Item Type"}, "latitude": { "anyOf": [{"type": "number"}, {"type": "null"}], "title": "Latitude" }, "longitude": { "anyOf": [{"type": "number"}, {"type": "null"}], "title": "Longitude" }, "location": { "anyOf": [{"type": "string"}, {"type": "null"}], "title": "Location" } }, "type": "object", "required": ["name", "date_created", "width", "height", "file_location", "file_size", "item_type"], "title": "MetadataModel" }
♻️ Duplicate comments (3)
backend/app/routes/images.py (2)
95-108: Performance issue: fetching all images to find one.Line 103 calls
db_get_all_images()to find a single image, which is highly inefficient and will cause performance degradation as the image collection grows. This issue was previously identified in an earlier review.Please refer to the previous review comment that proposed implementing
db_get_image_by_id()function to address this issue.
120-138: Performance issue: fetching all images to find one.Lines 123-124 fetch all images from the database to find a single image by ID, which is highly inefficient. This issue was previously identified in an earlier review.
Please refer to the previous review comment that proposed using a dedicated
db_get_image_by_id()function to address this issue.docs/backend/backend_python/openapi.json (1)
860-898: Missing response schema for toggle-favourite endpoint.Line 882 defines the 200 response schema as an empty object
{}, but the backend returns{"success": bool, "image_id": str, "isFavourite": bool}. This issue was previously identified in an earlier review.Please refer to the previous review comment that proposed adding a
ToggleFavouriteResponseschema to properly document the response structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/database/images.py(5 hunks)backend/app/routes/images.py(4 hunks)docs/backend/backend_python/openapi.json(8 hunks)frontend/src/pages/Home/Home.tsx(0 hunks)frontend/src/types/Media.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/pages/Home/Home.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/database/images.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/routes/images.py (2)
backend/app/database/images.py (2)
db_toggle_image_favourite_status(384-415)db_get_all_images(119-199)backend/app/utils/images.py (1)
image_util_parse_metadata(493-510)
🪛 Biome (2.1.2)
docs/backend/backend_python/openapi.json
[error] 2146-2146: unexpected character <
(parse)
[error] 2146-2146: unexpected character <
(parse)
[error] 2146-2146: unexpected character <
(parse)
[error] 2146-2146: unexpected character <
(parse)
[error] 2146-2146: unexpected character <
(parse)
[error] 2146-2146: unexpected character <
(parse)
[error] 2146-2146: String values must be double quoted.
(parse)
[error] 2147-2147: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2147-2147: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2183-2184: unexpected character =
(parse)
[error] 2184-2184: unexpected character =
(parse)
[error] 2184-2184: unexpected character =
(parse)
[error] 2184-2184: unexpected character =
(parse)
[error] 2184-2184: unexpected character =
(parse)
[error] 2184-2184: unexpected character =
(parse)
[error] 2184-2184: unexpected character =
(parse)
[error] 2195-2195: unexpected character >
(parse)
[error] 2195-2195: unexpected character >
(parse)
[error] 2195-2195: unexpected character >
(parse)
[error] 2195-2195: unexpected character >
(parse)
[error] 2195-2195: unexpected character >
(parse)
[error] 2195-2195: unexpected character >
(parse)
[error] 2195-2195: expected , but instead found 589
Remove 589
(parse)
[error] 2195-2195: expected , but instead found d71f3ac56247ffd133813c163893bd7762069
Remove d71f3ac56247ffd133813c163893bd7762069
(parse)
[error] 2196-2196: expected , but instead found }
Remove }
(parse)
[error] 2197-2197: Expected a property but instead found '{'.
Expected a property here.
(parse)
[error] 2200-2200: expected , but instead found ]
Remove ]
(parse)
[error] 2200-2201: unexpected character <
(parse)
[error] 2201-2201: unexpected character <
(parse)
[error] 2201-2201: unexpected character <
(parse)
[error] 2201-2201: unexpected character <
(parse)
[error] 2201-2201: unexpected character <
(parse)
[error] 2201-2201: unexpected character <
(parse)
[error] 2201-2201: unexpected character <
(parse)
[error] 2201-2201: String values must be double quoted.
(parse)
[error] 2202-2202: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2202-2202: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2202-2202: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2202-2203: unexpected character =
(parse)
[error] 2203-2203: unexpected character =
(parse)
[error] 2203-2203: unexpected character =
(parse)
[error] 2203-2203: unexpected character =
(parse)
[error] 2203-2203: unexpected character =
(parse)
[error] 2203-2203: unexpected character =
(parse)
[error] 2203-2203: unexpected character =
(parse)
[error] 2258-2258: unexpected character >
(parse)
[error] 2258-2258: unexpected character >
(parse)
[error] 2258-2258: unexpected character >
(parse)
[error] 2258-2258: unexpected character >
(parse)
[error] 2258-2258: unexpected character >
(parse)
[error] 2258-2258: unexpected character >
(parse)
[error] 2258-2258: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2258-2258: String values must be double quoted.
(parse)
[error] 2259-2260: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2261-2261: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2261-2261: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2261-2261: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2261-2261: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2262-2262: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2262-2262: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2262-2263: unexpected character <
(parse)
[error] 2263-2263: unexpected character <
(parse)
[error] 2263-2263: unexpected character <
(parse)
[error] 2263-2263: unexpected character <
(parse)
[error] 2263-2263: unexpected character <
(parse)
[error] 2263-2263: unexpected character <
(parse)
[error] 2263-2263: unexpected character <
(parse)
[error] 2263-2263: expected , but instead found HEAD
Remove HEAD
(parse)
[error] 2264-2264: expected , but instead found "id"
Remove "id"
(parse)
[error] 2262-2271: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2271-2271: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2272-2272: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2272-2272: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2272-2272: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2272-2273: unexpected character =
(parse)
[error] 2273-2273: unexpected character =
(parse)
[error] 2273-2273: unexpected character =
(parse)
[error] 2273-2273: unexpected character =
(parse)
[error] 2273-2273: unexpected character =
(parse)
[error] 2273-2273: unexpected character =
(parse)
[error] 2273-2273: unexpected character =
(parse)
[error] 2282-2283: unexpected character >
(parse)
[error] 2283-2283: unexpected character >
(parse)
[error] 2283-2283: unexpected character >
(parse)
[error] 2283-2283: unexpected character >
(parse)
[error] 2283-2283: unexpected character >
(parse)
[error] 2283-2283: unexpected character >
(parse)
[error] 2283-2283: unexpected character >
(parse)
[error] 2283-2283: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2283-2283: String values must be double quoted.
(parse)
[error] 2486-2486: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2486-2486: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2486-2498: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2498-2498: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
frontend/src/types/Media.ts
[error] 21-21: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 21-23: Expected a statement but instead found '=======
metadata?: ImageMetadata'.
Expected a statement here.
(parse)
[error] 24-24: numbers cannot be followed by identifiers directly after
an identifier cannot appear here
(parse)
🪛 GitHub Actions: PR Check Build
frontend/src/types/Media.ts
[error] 19-19: TS1185: Merge conflict marker encountered.
⏰ 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: Backend Tests
| path: str | ||
| folder_id: str | ||
| thumbnailPath: str | ||
| metadata: str |
There was a problem hiding this comment.
Type mismatch in ImageInfoResponse.
The metadata field is typed as str, but the ImageData model uses MetadataModel (a structured Pydantic model). This inconsistency means:
ImageInfoResponsereturns raw string metadataImageDatain/endpoint returns parsed structured metadata- Frontend consumers must handle two different metadata formats
Align the type to match ImageData:
class ImageInfoResponse(BaseModel):
id: str
path: str
folder_id: str
thumbnailPath: str
- metadata: str
+ metadata: MetadataModel
isTagged: bool
isFavourite: bool
tags: Optional[List[str]] = NoneThen update line 134 to parse the metadata:
return ImageInfoResponse(
id=image["id"],
path=image["path"],
folder_id=image["folder_id"],
thumbnailPath=image["thumbnailPath"],
- metadata=image["metadata"],
+ metadata=image_util_parse_metadata(image["metadata"]),
isTagged=image["isTagged"],
isFavourite=image.get("isFavourite", False),
tags=image.get("tags", [])
)📝 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.
| metadata: str | |
| # --- In backend/app/routes/images.py --- | |
| # 1) Update the response model | |
| class ImageInfoResponse(BaseModel): | |
| id: str | |
| path: str | |
| folder_id: str | |
| thumbnailPath: str | |
| metadata: MetadataModel | |
| isTagged: bool | |
| isFavourite: bool | |
| tags: Optional[List[str]] = None | |
| # 2) Parse the raw metadata when constructing the response | |
| @router.get("/info/{image_id}", response_model=ImageInfoResponse) | |
| def get_image_info(image_id: str): | |
| image = db_get_image_by_id(image_id) | |
| if not image: | |
| raise HTTPException(status_code=404, detail="Image not found") | |
| return ImageInfoResponse( | |
| id=image["id"], | |
| path=image["path"], | |
| folder_id=image["folder_id"], | |
| thumbnailPath=image["thumbnailPath"], | |
| metadata=image_util_parse_metadata(image["metadata"]), | |
| isTagged=image["isTagged"], | |
| isFavourite=image.get("isFavourite", False), | |
| tags=image.get("tags", []) | |
| ) |
🤖 Prompt for AI Agents
In backend/app/routes/images.py around line 115 and again at line 134, change
the ImageInfoResponse.metadata type from str to MetadataModel (import it if not
already), and at line 134 parse the stored metadata string into that model
before returning (e.g., if record.metadata is a JSON string use
MetadataModel.parse_raw(record.metadata), otherwise construct via
MetadataModel(**record.metadata)); ensure the response uses the MetadataModel
instance so both endpoints return the same structured metadata.
There was a problem hiding this comment.
@Vaibhaviitian
Fix this coderabbit suggestion.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/pages/Home/MyFav.tsx (1)
58-58: Use English for code comments.The inline comment is in Hindi. This was previously flagged in past reviews.
Apply this diff:
- // Sirf favourite wali images ke liye + // Filter to show only favourite images const favouriteImages = displayImages.filter(
🧹 Nitpick comments (1)
frontend/src/pages/Home/MyFav.tsx (1)
67-108: Nice empty state UX!The empty state provides clear guidance and a call-to-action button, which is excellent for user experience.
Optional improvement: Consider using a route constant instead of the hardcoded
'/'at line 100 for better maintainability:+import { ROUTES } from '@/constants/routes'; + // ... - onClick={() => navigate('/')} + onClick={() => navigate(ROUTES.HOME)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/Media/ImageCard.tsx(2 hunks)frontend/src/components/Media/MediaView.tsx(6 hunks)frontend/src/pages/Home/MyFav.tsx(1 hunks)frontend/src/types/Media.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/Media/MediaView.tsx
- frontend/src/components/Media/ImageCard.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/Home/MyFav.tsx (10)
frontend/src/features/imageSelectors.ts (2)
selectIsImageViewOpen(24-27)selectImages(5-7)frontend/src/app/store.ts (1)
RootState(22-22)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/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)frontend/src/types/Media.ts (1)
Image(13-23)frontend/src/features/imageSlice.ts (1)
setImages(22-26)frontend/src/components/Media/ImageCard.tsx (1)
ImageCard(20-116)frontend/src/components/Media/MediaView.tsx (1)
MediaView(27-218)
⏰ 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 (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Backend Tests
🔇 Additional comments (6)
frontend/src/types/Media.ts (1)
20-20: LGTM! The merge conflict has been resolved correctly.The
isFavouriteoptional property has been added successfully, retaining the structuredmetadatafield from the other branch. The optional flag enables backward compatibility and aligns with the backend changes and UI components.frontend/src/pages/Home/MyFav.tsx (5)
1-14: LGTM! Imports are well-organized.All necessary dependencies are imported and properly used throughout the component.
15-29: LGTM! Component setup and query configuration are correct.The
usePictoQueryhook is properly configured with appropriate query key, function, and enablement logic that avoids redundant fetches when search is active.
31-51: LGTM! Fetch lifecycle is handled correctly.The effect properly manages loading states, error handling, and success dispatches. The conditional logic ensures it only runs when search is inactive, avoiding conflicts with search results.
57-66: Verify the search integration behavior on the Favourites page.The current implementation filters search results when
isSearchActiveis true (line 57), and the title changes to "Face Search Results" on the Favourites page (lines 62-65). This might confuse users who navigate to the Favourites page expecting to see all their favourites, not filtered search results.Consider whether the Favourites page should:
- Always display all favourite images regardless of search state, or
- Show filtered search results when a search is active (current behavior)
If option 1 is preferred, the search state should be cleared when navigating to the Favourites page, or the page should ignore search state entirely.
110-131: LGTM! The main render logic is correct.The grid properly maps over
favouriteImages, andMediaViewreceives the same array, ensuring index consistency for navigation. The modal correctly limits navigation to favourite images only.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/app/database/face_clusters.py (1)
198-200: Unrelated formatting change: parentheses add no value here.This change wraps the return type in parentheses but doesn't improve readability or align with the PR's favorites feature. The line isn't long enough to require breaking, and this formatting is inconsistent with other functions in the file (e.g.,
db_get_all_clusters()on Line 108 anddb_get_cluster_by_id()on Line 80 use unwrapped return types).Consider removing this change or applying consistent formatting across all return types in a separate, dedicated formatting PR.
backend/app/database/faces.py (2)
237-239: Unrelated formatting change: parentheses add no value here.Similar to
backend/app/database/face_clusters.py, this change wraps the return type in parentheses without improving readability or contributing to the PR's favorites feature. The formatting is also inconsistent with other functions in this file (e.g.,db_get_faces_unassigned_clusters()on Line 212 uses an unwrapped return type).Consider removing this change or applying consistent formatting across all return types in a separate, dedicated formatting PR.
274-274: Trailing comma is valid but unrelated to PR objectives.While trailing commas are a valid Python convention that can make diffs cleaner when adding parameters, this change is unrelated to the favorites functionality being implemented in this PR.
Consider deferring such formatting changes to a dedicated formatting PR to keep this PR focused on the feature implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/database/face_clusters.py(1 hunks)backend/app/database/faces.py(2 hunks)backend/app/database/folders.py(1 hunks)backend/app/database/images.py(5 hunks)backend/app/routes/albums.py(1 hunks)backend/app/routes/images.py(4 hunks)backend/app/utils/images.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/app/routes/albums.py
- backend/app/database/folders.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/database/images.py
- backend/app/routes/images.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 (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
🔇 Additional comments (2)
backend/app/utils/images.py (2)
103-103: AI summary inconsistency.The AI-generated summary states that return type annotations (
-> Noneand-> Dict[str, int]) were added to these functions, but the annotations were already present (lines 103 and 266 are not marked as changed). Only the trailing commas on the preceding parameter lines (102 and 265) are new.Also applies to: 266-266
102-102: Trailing commas are good Python style.Adding trailing commas to function parameters follows Python best practices and improves future diff readability when parameters are extended.
However, note that these formatting changes are unrelated to the PR's stated objective of adding favorites functionality. Consider whether such unrelated stylistic changes should be included in feature PRs or submitted separately.
Also applies to: 265-265
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/database/images.py (1)
385-417: Reuse_connect()for consistent connection configuration.Every other DB helper relies on
_connect()soPRAGMA foreign_keys = ONstays enforced. This helper bypasses that, creating a connection without FK enforcement (and duplicating setup logic). Switching to_connect()keeps the behavior consistent.-def db_toggle_image_favourite_status(image_id: str) -> bool: - conn = sqlite3.connect(DATABASE_PATH) +def db_toggle_image_favourite_status(image_id: str) -> bool: + conn = _connect()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/database/images.py(5 hunks)
⏰ 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 (windows-latest)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Backend Tests
|
@rahulharpal1603 sir please review this |
|
@Vaibhaviitian |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
backend/app/routes/images.py (4)
101-102: Refactor to fetch single image instead of all images.This endpoint fetches all images from the database just to find one by ID, causing severe performance degradation as the collection grows. This issue was flagged in a previous review.
See the previous review comment for the recommended solution of creating a dedicated
db_get_image_by_id()function.
120-120: Type mismatch in ImageInfoResponse metadata field.The
metadatafield is typed asstr, butImageDataat line 31 usesMetadataModel. This inconsistency was flagged in a previous review and causes clients to handle two different metadata formats.See the previous review comment for the recommended fix to align with
MetadataModeland parse the metadata at line 140.
129-130: Use dedicated single-image query instead of fetching all images.Lines 129-130 fetch all images from the database to find one by ID, which is highly inefficient. This issue was flagged in a previous review.
See the previous review comment for the recommended solution using a dedicated
db_get_image_by_id()function.
140-140: Metadata type mismatch with ImageInfoResponse.Line 140 returns raw
image["metadata"], but according to the relevant code snippet fromdb_get_all_images(), this is already a dict. However,ImageInfoResponse.metadataat line 120 is typed asstr, creating a mismatch. Additionally, this differs from line 61 where metadata is properly parsed usingimage_util_parse_metadata().See the previous review comment on line 120 for the complete fix.
🧹 Nitpick comments (2)
backend/app/routes/images.py (2)
7-7: Consider combining imports from the same module.Lines 3 and 7 both import from
app.database.images. For consistency and readability, combine them into a single import statement.Apply this diff:
-from app.database.images import db_get_all_images +from app.database.images import db_get_all_images, db_toggle_image_favourite_status from app.schemas.images import ErrorResponse from app.utils.images import image_util_parse_metadata from pydantic import BaseModel -from app.database.images import db_toggle_image_favourite_status
93-93: Consider adding a response model for consistency.The endpoint returns a structured response but doesn't declare a response model, unlike
/info/{image_id}at line 126. Adding a Pydantic model improves API documentation and type safety.Define a response model:
class ToggleFavouriteResponse(BaseModel): success: bool image_id: str isFavourite: boolThen update the decorator:
-@router.post("/toggle-favourite") +@router.post("/toggle-favourite", response_model=ToggleFavouriteResponse) def toggle_favourite(req: ToggleFavouriteRequest):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/routes/images.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/routes/images.py (1)
backend/app/database/images.py (2)
db_toggle_image_favourite_status(384-417)db_get_all_images(119-199)
🔇 Additional comments (3)
backend/app/routes/images.py (3)
33-33: LGTM!The addition of the
isFavouritefield toImageDatacorrectly exposes the favourite status in the API response.
63-63: LGTM!Safely retrieves
isFavouritewith a sensible default value.
89-90: LGTM!The
ToggleFavouriteRequestmodel is appropriately defined for the toggle endpoint.
DecodeX15
force-pushed
the
adding_functionality_for_share_and_addto_favourite
branch
from
69989b4 to
d18aa76
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
frontend/src/components/Media/MediaView.tsx (4)
48-49: Sync isfav with currentImage to prevent stale icon after navigation.Without syncing, the favourite icon lags when moving between images.
-const [isfav, setIsfav] = useState(currentImage?.isFavourite || false); +const [isfav, setIsfav] = useState(currentImage?.isFavourite || false); + +// keep local flag in sync with the image being viewed +useEffect(() => { + setIsfav(currentImage?.isFavourite || false); +}, [currentImage?.isFavourite]);Also update imports:
-import { useState, useCallback, useMemo } from 'react'; +import { useState, useCallback, useMemo, useEffect } from 'react';
115-120: Remove double toggle; delegate to the unified handler.Local flip plus server update causes flicker and race risk.
-const handleToggleFavorite = useCallback(() => { - if (currentImage) { - setIsfav((prev) => !prev); - handle_favourite_toggle(); - } -}, [currentImage, isfav]); +const handleToggleFavorite = useCallback(() => { + if (currentImage) { + handleFavouriteToggle(); + } +}, [currentImage, handleFavouriteToggle]);
73-102: Consolidate favourite toggle: optimistic update + rollback, no alerts, no duplicate local toggles.Current flow mixes alerts, console logs, and a separate local toggle, which can desync UI and feels jarring. Make the handler own the optimistic update, rollback on error, and remove UI‑blocking alerts.
-// handling toogle_favvvvv -const handle_favourite_toggle = async () => { - // console.log('processing ..'); - if (!currentImage) return; - if (import.meta.env.VITE_BACKEND_URL === undefined) { - alert('Backend URL is not defined'); - return; - } - try { - const res = await axios.post( - `${import.meta.env.VITE_BACKEND_URL}${imagesEndpoints.setfavourite}`, - { - image_id: currentImage?.id, - }, - ); - if (res.data.success) { - setIsfav(res.data.isFavourite); - await queryclient.invalidateQueries({ queryKey: ['images'] }); - res?.data?.isFavourite - ? alert('Add to Favourite') - : alert('Removed from Favourite'); - console.log('toggled'); - toggleFavorite(currentImage?.path || ''); - } - console.log(res); - } catch (error) { - alert('Error toggling favourite'); - console.log(error); - } -}; +// toggle favourite with optimistic UI and rollback on failure +const handleFavouriteToggle = async () => { + if (!currentImage?.id) return; + if (!import.meta.env.VITE_BACKEND_URL) return; + + const previous = isfav; + setIsfav(!previous); // optimistic + try { + const res = await axios.post( + `${import.meta.env.VITE_BACKEND_URL}${imagesEndpoints.setfavourite}`, + { image_id: currentImage.id }, + ); + if (res.data?.success) { + setIsfav(Boolean(res.data.isFavourite)); + await queryClient.invalidateQueries({ queryKey: ['images'] }); + // TODO: replace with your non-blocking notification (toast/snackbar) + } else { + setIsfav(previous); // rollback on unexpected payload + } + } catch (error) { + setIsfav(previous); // rollback on error + // TODO: replace with your non-blocking error notification + } +};Optional (recommended): switch to react-query useMutation for pending/disable and cleaner invalidation. Example:
const toggleFav = useMutation({ mutationFn: (id: number) => axios.post(`${import.meta.env.VITE_BACKEND_URL}${imagesEndpoints.setfavourite}`, { image_id: id }) .then(r => r.data), onMutate: async () => { const prev = isfav; setIsfav(!prev); return { prev }; }, onError: (_e, _v, ctx) => setIsfav(ctx?.prev ?? false), onSuccess: (data) => setIsfav(Boolean(data.isFavourite)), onSettled: () => queryClient.invalidateQueries({ queryKey: ['images'] }), });Based on learnings
47-49: Use a single source of truth for favourites; remove useFavorites and derive from images.The local favourites array (path-based) competes with server isFavourite (id-based). This can drift.
-import { useFavorites } from '@/hooks/useFavorites'; +// remove local favourites; derive from images' isFavourite-const { favorites, toggleFavorite } = useFavorites(); -const [isfav, setIsfav] = useState(currentImage?.isFavourite || false); +const [isfav, setIsfav] = useState(currentImage?.isFavourite || false); +const favorites = useMemo( + () => images.filter((img) => img.isFavourite).map((img) => img.path), + [images], +);- toggleFavorite(currentImage?.path || ''); + // removed: local favourites toggle; server is the source of truth- favorites={favorites} + favorites={favorites}And drop the unused import of useFavorites at the top of the file.
Also applies to: 95-96, 211-212
🧹 Nitpick comments (1)
frontend/src/components/Media/MediaView.tsx (1)
139-139: Remove commented debug code.-// console.log(currentImage);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Media/MediaView.tsx(7 hunks)frontend/src/pages/Home/Home.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/Home/Home.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/Media/MediaView.tsx (3)
frontend/src/hooks/useImageViewControls.ts (1)
useImageViewControls(11-94)frontend/src/hooks/useFavorites.ts (1)
useFavorites(3-28)frontend/src/api/apiEndpoints.ts (1)
imagesEndpoints(1-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (windows-latest)
| const [showInfo, setShowInfo] = useState(false); | ||
| const [showThumbnails, setShowThumbnails] = useState(false); | ||
|
|
||
| const queryclient = useQueryClient(); |
There was a problem hiding this comment.
🧩 Analysis chain
Consistent naming and key verification.
Rename queryclient → queryClient; verify that ['images'] matches actual query keys.
-const queryclient = useQueryClient();
+const queryClient = useQueryClient();- await queryclient.invalidateQueries({ queryKey: ['images'] });
+ await queryClient.invalidateQueries({ queryKey: ['images'] });Run to confirm keys used for image lists/favourites:
Also applies to: 90-90
🏁 Script executed:
#!/bin/bash
# Find react-query usages and query keys related to images/favourites
rg -n --type=ts --type=tsx -C2 "\buse(Query|SuspenseQuery|InfiniteQuery|Mutation)\b|queryKey\s*:\s*\["
# Specifically list all ['images'] keys
rg -n --type=ts --type=tsx -C1 "queryKey\s*:\s*\[\s*'images'\s*\]"Length of output: 55
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for React Query patterns without explicit tsx type
rg -n --type=typescript -C2 "useQuery|useSuspenseQuery|useInfiniteQuery|useMutation|queryKey"
# Alternative: search using glob pattern
rg -n -g "*.tsx" -g "*.ts" -C1 "queryKey\s*:\s*\["
# Find all ['images'] key usages
rg -n -g "*.tsx" -g "*.ts" "'images'"
Length of output: 3175
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all queryclient usages in MediaView.tsx
rg -n -g "*.tsx" -g "*.ts" "queryclient" frontend/src/components/Media/MediaView.tsx
Length of output: 116
Rename queryclient to queryClient and verify query keys.
The variable naming must follow camelCase convention. The query key ['images'] is verified as correct and used consistently across MyFav.tsx, Home.tsx, and AITagging.tsx for image list queries.
Update required at lines 44 and 90 in frontend/src/components/Media/MediaView.tsx:
-const queryclient = useQueryClient();
+const queryClient = useQueryClient();-await queryclient.invalidateQueries({ queryKey: ['images'] });
+await queryClient.invalidateQueries({ queryKey: ['images'] });📝 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 queryclient = useQueryClient(); | |
| const queryClient = useQueryClient(); |
🤖 Prompt for AI Agents
In frontend/src/components/Media/MediaView.tsx around lines 44 and 90, rename
the variable queryclient to queryClient (camelCase) and update all references to
use queryClient; also verify and ensure the query key ['images'] is used
consistently for image list queries (matching MyFav.tsx, Home.tsx, and
AITagging.tsx) when calling useQuery/useMutation or accessing cached data so no
stale or mismatched keys remain.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
frontend/src/components/Media/MediaView.tsx (4)
48-48: Sync local isfav when currentImage changes.
Local state becomes stale after navigation; add an effect to mirror currentImage?.isFavourite.// add near other hooks useEffect(() => { setIsfav(currentImage?.isFavourite ?? false); }, [currentImage?.isFavourite]);Also ensure the import line includes useEffect.
44-44: Rename queryclient → queryClient.
Follow camelCase and update references (invalidateQueries call).- const queryclient = useQueryClient(); + const queryClient = useQueryClient(); ... - await queryclient.invalidateQueries({ queryKey: ['images'] }); + await queryClient.invalidateQueries({ queryKey: ['images'] });Verify the query key matches list keys used elsewhere.
#!/bin/bash rg -n --type=ts --type=tsx -C2 "queryKey\s*:\s*\[" | rg -n "images"Also applies to: 91-91
116-121: Remove redundant local toggle and fix dependencies.Avoid double-toggling; just call the handler. Update deps.
- const handleToggleFavorite = useCallback(() => { - if (currentImage) { - setIsfav((prev) => !prev); - handle_favourite_toggle(); - } - }, [currentImage, isfav]); + const handleToggleFavorite = useCallback(() => { + if (currentImage) { + handleFavouriteToggle(); + } + }, [currentImage, handleFavouriteToggle]);
73-104: Consolidate favourite toggle: optimistic update + rollback; remove alerts and duplicate local hook.Current handler mixes alert UX, redundant local toggleFavorite, dead check for undefined backend URL, and no rollback on error.
Apply this focused rewrite:
- // handling toogle_favvvvv - const handle_favourite_toggle = async () => { - // console.log('processing ..'); - if (!currentImage) return; - const backendurl = getBackendUrl(); - if (backendurl === undefined) { - alert('Backend URL is not defined'); - return; - } - try { - const res = await axios.post( - `${backendurl}${imagesEndpoints.setfavourite}`, - { - image_id: currentImage?.id, - }, - ); - if (res.data.success) { - setIsfav(res.data.isFavourite); - await queryclient.invalidateQueries({ queryKey: ['images'] }); - res?.data?.isFavourite - ? alert('Add to Favourite') - : alert('Removed from Favourite'); - console.log('toggled'); - toggleFavorite(currentImage?.path || ''); - } - console.log(res); - } catch (error) { - alert('Error toggling favourite'); - console.log(error); - } - }; + // Favourite toggle with optimistic UI and rollback + const handleFavouriteToggle = async () => { + if (!currentImage?.id) return; + const backendUrl = getBackendUrl(); + const prev = isfav; + setIsfav(!prev); + try { + const res = await axios.post( + `${backendUrl}${imagesEndpoints.setfavourite}`, + { image_id: currentImage.id }, + ); + if (res?.data?.success) { + setIsfav(Boolean(res.data.isFavourite)); + await queryClient.invalidateQueries({ queryKey: ['images'] }); + } else { + setIsfav(prev); + } + } catch (err) { + setIsfav(prev); + console.error('Failed to toggle favourite', err); + } + };Follow-ups:
- Drop toggleFavorite/useFavorites from this component to avoid dual sources of truth; rely on server + cache invalidation.
- Replace alerts with your app’s non-blocking notification (toast/snackbar).
- Consider a useMutation wrapper for better status handling.
🧹 Nitpick comments (3)
frontend/src/utils/env.ts (1)
1-3: Validate VITE_BACKEND_URL and normalize.Return type is string but value can be missing at runtime; also normalize trailing slash to avoid double “//”.
Apply this diff:
-export const getBackendUrl = (): string => { - return import.meta.env.VITE_BACKEND_URL ; -}; +export const getBackendUrl = (): string => { + const url = (import.meta as any)?.env?.VITE_BACKEND_URL as string | undefined; + if (!url) throw new Error('VITE_BACKEND_URL is not defined'); + return url.replace(/\/$/, ''); +};frontend/jest.setup.ts (1)
3-5: Remove leftover commented code.Clean up “REMOVE THESE LINES” block to avoid confusion in setup files.
frontend/src/components/Media/MediaView.tsx (1)
19-19: Type check: backend URL guard is ineffective with current typing.getBackendUrl returns string; comparing to undefined is dead code. Either throw inside getBackendUrl (preferred) or change to if (!backendurl).
Also applies to: 78-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/jest.config.ts(1 hunks)frontend/jest.setup.ts(2 hunks)frontend/src/components/Media/MediaView.tsx(6 hunks)frontend/src/utils/env.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/Media/MediaView.tsx (4)
frontend/src/hooks/useImageViewControls.ts (1)
useImageViewControls(11-94)frontend/src/hooks/useFavorites.ts (1)
useFavorites(3-28)frontend/src/utils/env.ts (1)
getBackendUrl(1-3)frontend/src/api/apiEndpoints.ts (1)
imagesEndpoints(1-4)
⏰ 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 (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Backend Tests
| // Direct value do | ||
| Object.defineProperty(global, 'import', { | ||
| value: { | ||
| meta: { | ||
| env: { | ||
| VITE_BACKEND_URL: 'http://localhost:8000' // Direct value | ||
| } | ||
| } | ||
| } | ||
| }); No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Avoid brittle import.meta polyfill; mock the env util instead.
Defining global.import is unreliable for module-scoped import.meta. Prefer mocking '@/utils/env'.
Replace the block with:
jest.mock('@/utils/env', () => ({
getBackendUrl: () => 'http://localhost:8000',
}));If you must polyfill, attach to globalThis (still brittle):
Object.defineProperty(globalThis as any, 'import', {
value: { meta: { env: { VITE_BACKEND_URL: 'http://localhost:8000' } } },
});Add a search to ensure no other direct import.meta usages leak into tests.
🏁 Script executed:
#!/bin/bash
rg -n --type=ts --type=tsx -C1 "import\.meta"Length of output: 27
I'll verify the codebase to assess the review comment. The previous script had an error with file type specification. Let me correct it and gather additional information.
🏁 Script executed:
#!/bin/bash
# Search for import.meta usage in TypeScript/JavaScript files
rg -n "import\.meta" --type=ts
# Also search without type restriction to catch all occurrences
echo "---"
echo "Searching without type restriction:"
rg -n "import\.meta"Length of output: 487
🏁 Script executed:
#!/bin/bash
# Check if @/utils/env file exists
fd "env\.(ts|tsx|js|jsx)$" --path "*utils*"
echo "---"
# Also check the jest.setup.ts file for context
echo "jest.setup.ts content:"
find . -name "jest.setup.ts" -type f -exec cat {} \;Length of output: 965
🏁 Script executed:
#!/bin/bash
# Search for any mocking of @/utils/env in the codebase
rg "@/utils/env|utils/env" -nLength of output: 164
Review comment is valid; refactoring suggestion is necessary.
The verification confirms the review comment is correct:
- @/utils/env exists and exports functions using
import.meta.env(specificallygetBackendUrl()which is actively imported inMediaView.tsx) - The polyfill on
global.importis fundamentally brittle: it doesn't affect module-scopedimport.metain ES modules - Other utilities like
isProd.tsalso depend onimport.meta.envdirectly, so the polyfill won't help them - The commented-out code in jest.setup.ts (lines 4-5) shows the recommended mocking approach was already considered
The recommendation to mock @/utils/env instead is the correct solution. The current polyfill approach will fail for any code that actually accesses import.meta.env.
sir please review it and tell me |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/Media/ImageCard.tsx (1)
26-34: Critical: Favorite toggle has no backend persistence.The
handle_favourite_togglefunction only updates local component state (isFavorite) without calling the backend API to persist the change. Additionally, the UI logic creates a state inconsistency: the button styling and icon rendering (lines 74-84) readimage.isFavouritefrom props, but the handler updates the disconnected localisFavoritestate. This means:
- Clicking the favorite button won't persist to the database
- The UI won't reflect the change because it reads from props, not local state
- State becomes stale when the image prop updates from other sources
Past review comments flagged this exact issue as critical but it remains unresolved.
You should remove the local state entirely and integrate with the backend toggle API. Based on the
useToggleFavhook introduced elsewhere in this PR:- const [isFavorite, setIsFavorite] = useState(false); + const { toggleFavourite } = useToggleFav(); - const handle_favourite_toggle = () => { - setIsFavorite(!isFavorite); + const handle_favourite_toggle = (e: React.MouseEvent) => { + e.stopPropagation(); // Prevent card click + if (image.id) { + toggleFavourite(image.id); + } };And update the button click handler:
- onClick={handle_favourite_toggle} + onClick={handle_favourite_toggle}The
useToggleFavhook already handles the backend call and query invalidation, so the UI will re-render with updated data from Redux after the mutation succeeds.
♻️ Duplicate comments (9)
backend/app/database/images.py (2)
66-66: Critical: Database migration still missing for existing deployments.Adding
isFavouriteonly in theCREATE TABLE IF NOT EXISTSstatement will causesqlite3.OperationalError: no such column: i.isFavouriteon upgraded instances whendb_get_all_imagesexecutes. You must add migration logic to detect and add the missing column for existing databases.Apply this diff to add the migration:
cursor.execute( """ CREATE TABLE IF NOT EXISTS images ( id TEXT PRIMARY KEY, path VARCHAR UNIQUE, folder_id INTEGER, thumbnailPath TEXT UNIQUE, metadata TEXT, isTagged BOOLEAN DEFAULT 0, isFavourite BOOLEAN DEFAULT 0, FOREIGN KEY (folder_id) REFERENCES folders(folder_id) ON DELETE CASCADE ) """ ) + + # Migration: Add isFavourite column if it doesn't exist + cursor.execute("PRAGMA table_info(images)") + existing_columns = {column[1] for column in cursor.fetchall()} + if "isFavourite" not in existing_columns: + cursor.execute( + "ALTER TABLE images ADD COLUMN isFavourite BOOLEAN DEFAULT 0" + ) + conn.commit()
399-421: Refactor to use codebase conventions.The function still has inconsistencies with the rest of the codebase:
- Uses
sqlite3.connect()directly instead of_connect()helper (line 400)- Missing docstring (all other functions in this module have them)
Apply this diff:
+def db_toggle_image_favourite_status(image_id: str) -> bool: + """ + Toggle the isFavourite status for a specific image. + + Args: + image_id: ID of the image to toggle + + Returns: + True if toggle was successful, False otherwise + """ -def db_toggle_image_favourite_status(image_id: str) -> bool: - conn = sqlite3.connect(DATABASE_PATH) + conn = _connect() cursor = conn.cursor() try: cursor.execute("SELECT id FROM images WHERE id = ?", (image_id,)) if not cursor.fetchone(): return False cursor.execute( """ UPDATE images SET isFavourite = CASE WHEN isFavourite = 1 THEN 0 ELSE 1 END WHERE id = ? """, (image_id,), ) conn.commit() return cursor.rowcount > 0 except Exception as e: logger.error(f"Database error: {e}") conn.rollback() return False finally: conn.close()backend/app/routes/images.py (2)
98-119: Critical: Multiple unfixed issues in toggle_favourite endpoint.This endpoint still has three unresolved issues from previous reviews:
Performance issue (lines 108-110): Fetching all images to find one is highly inefficient and will degrade as the collection grows.
Potential AttributeError (line 114): If
imageisNone, callingimage.get(...)will raiseAttributeError.Exception handling bug (lines 117-119): The broad
except Exceptionwill catch theHTTPExceptionraised at line 104, converting a legitimate 404 into a 500 error.Apply this diff to fix all three issues:
+from app.database.images import db_toggle_image_favourite_status, db_get_image_by_id + @router.post("/toggle-favourite") def toggle_favourite(req: ToggleFavouriteRequest): image_id = req.image_id try: success = db_toggle_image_favourite_status(image_id) if not success: raise HTTPException( status_code=404, detail="Image not found or failed to toggle" ) - # Fetch updated status to return - image = next( - (img for img in db_get_all_images() if img["id"] == image_id), None - ) + image = db_get_image_by_id(image_id) + if not image: + raise HTTPException(status_code=404, detail="Image not found after toggle") + return { "success": True, "image_id": image_id, "isFavourite": image.get("isFavourite", False), } + except HTTPException: + raise except Exception as e: logger.error(f"error in /toggle-favourite route: {e}") raise HTTPException(status_code=500, detail=f"Internal server error: {e}")Note: You'll need to implement
db_get_image_by_idinbackend/app/database/images.pyas suggested in the previous review comment on that file.
122-130: Remove unused ImageInfoResponse model.This model was added for debugging purposes and has no corresponding endpoint in the codebase. Based on past review comments, it should be removed.
Apply this diff:
- - -class ImageInfoResponse(BaseModel): - id: str - path: str - folder_id: str - thumbnailPath: str - metadata: MetadataModel - isTagged: bool - isFavourite: bool - tags: Optional[List[str]] = Nonedocs/backend/backend_python/openapi.json (1)
890-928: Define proper response schema for toggle-favourite endpoint.The 200 response schema is an empty object
{}(line 912), but the backend actually returns{"success": bool, "image_id": str, "isFavourite": bool}. This mismatch will cause issues for API consumers and code generators.Add this schema to the
components/schemassection:"ToggleFavouriteResponse": { "properties": { "success": { "type": "boolean", "title": "Success" }, "image_id": { "type": "string", "title": "Image Id" }, "isFavourite": { "type": "boolean", "title": "Isfavourite" } }, "type": "object", "required": [ "success", "image_id", "isFavourite" ], "title": "ToggleFavouriteResponse" }Then update the endpoint response reference:
"200": { "description": "Successful Response", "content": { "application/json": { - "schema": {} + "schema": { + "$ref": "#/components/schemas/ToggleFavouriteResponse" + } } } },frontend/src/components/Media/MediaView.tsx (3)
149-149: Remove commented debug code.Commented-out
console.logstatements should be removed to keep the codebase clean.The maintainer explicitly requested: "Remove all the unnecessary comments from the files that you have changed."
As per coding guidelines.
const currentImagePath = currentImage.path; - // console.log(currentImage); const currentImageAlt = `image-${currentViewIndex}`;
52-52: Critical: Local favorite state not synced with current image.The
isfavstate is initialized fromcurrentImage?.isFavouritebut never updates when the user navigates to a different image (via next/previous buttons or thumbnails). This causes the favorite icon to display stale state after navigation.This critical issue was flagged in past reviews but remains unresolved.
Add a
useEffectto synchronizeisfavwith the current image:const [isfav, setIsfav] = useState(currentImage?.isFavourite || false); + + useEffect(() => { + setIsfav(currentImage?.isFavourite || false); + }, [currentImage?.isFavourite]);Alternatively, remove the local state entirely and use
currentImage?.isFavouritedirectly in the UI, updating it via Redux after the mutation succeeds.
110-115: Remove redundant optimistic state update.The
handleToggleFavoritefunction togglesisfavstate optimistically (line 112), then callshandle_favourite_toggle()which invokes the mutation. This creates redundant state updates and the dependency array incorrectly includesisfav.Past reviews flagged this issue but it remains unresolved.
Remove the redundant
setIsfavcall and fix the dependency array:const handleToggleFavorite = useCallback(() => { if (currentImage) { - setIsfav((prev) => !prev); handle_favourite_toggle(); } - }, [currentImage, isfav]); + }, [currentImage, handle_favourite_toggle]);Note: If you want optimistic updates with proper error handling, implement it inside
handle_favourite_togglewith rollback logic on failure. The current implementation has no rollback mechanism.frontend/src/components/Media/ImageCard.tsx (1)
73-85: Favorite button triggers card click event.The favorite button's
onClickhandler doesn't callstopPropagation, so clicking the heart icon will also trigger the card's mainonClickhandler (line 44), opening the image viewer. This makes the favorite button unusable.Past review comments from the maintainer noted: "This button is not working. When I try to click the heart icon, the image is opened."
Add event propagation control as shown in the previous comment, or inline:
- onClick={handle_favourite_toggle} + onClick={(e) => { + e.stopPropagation(); + handle_favourite_toggle(e); + }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/app/database/images.py(5 hunks)backend/app/routes/images.py(4 hunks)docs/backend/backend_python/openapi.json(6 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/components/Media/ImageCard.tsx(2 hunks)frontend/src/components/Media/MediaView.tsx(7 hunks)frontend/src/pages/Home/MyFav.tsx(1 hunks)frontend/src/types/Media.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/api/apiEndpoints.ts
- frontend/src/types/Media.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/components/Media/MediaView.tsx (2)
frontend/src/hooks/useFavorites.ts (1)
useFavorites(3-28)frontend/src/hooks/useToggleFav.ts (1)
useToggleFav(5-17)
frontend/src/pages/Home/MyFav.tsx (12)
frontend/src/features/imageSelectors.ts (2)
selectIsImageViewOpen(13-16)selectImages(5-7)frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(8-12)ChronologicalGallery(23-180)frontend/src/app/store.ts (1)
RootState(22-22)frontend/src/hooks/useQueryExtension.ts (1)
usePictoQuery(80-108)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(5-14)frontend/src/features/loaderSlice.ts (2)
showLoader(17-20)hideLoader(21-24)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)frontend/src/types/Media.ts (1)
Image(13-23)frontend/src/features/imageSlice.ts (1)
setImages(18-20)frontend/src/components/EmptyStates/EmptyGalleryState.tsx (1)
EmptyGalleryState(3-28)frontend/src/components/Timeline/TimelineScrollbar.tsx (1)
TimelineScrollbar(38-409)frontend/src/components/Media/MediaView.tsx (1)
MediaView(27-227)
backend/app/routes/images.py (1)
backend/app/database/images.py (2)
db_toggle_image_favourite_status(399-421)db_get_all_images(123-214)
⏰ 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 Build Check (ubuntu-22.04)
🔇 Additional comments (8)
backend/app/database/images.py (1)
147-147: LGTM: isFavourite properly integrated into query results.The additions to select
isFavourite, unpack it from results, and include it in the returned dictionaries follow the existing patterns forisTagged.Also applies to: 174-174, 190-190
backend/app/routes/images.py (2)
7-11: LGTM: Imports and logger properly set up.The new imports and logger initialization follow the existing codebase patterns.
36-36: LGTM: ImageData model updated correctly.The
isFavouritefield is properly added to the model and correctly populated from the database response with a safe default.Also applies to: 68-68
docs/backend/backend_python/openapi.json (3)
2131-2134: LGTM: ImageData schema properly updated.The
isFavouritefield is correctly added as a required boolean, matching the backend model structure.Also applies to: 2157-2158
2549-2561: LGTM: ToggleFavouriteRequest schema matches backend model.The request schema correctly defines
image_idas a required string field, matching the Pydantic model in the backend.
1120-1122: Minor OpenAPI cleanup looks correct.The simplification of the InputType parameter reference (removing the
allOfwrapper) is a valid cleanup, though unrelated to the favourites feature.frontend/src/pages/Home/MyFav.tsx (1)
57-59: Verify MediaView close behavior.The
handleCloseMediaViewfunction is empty with a comment stating "MediaView will handle closing via Redux". This is acceptable sinceMediaViewinternally dispatchescloseImageView()(as seen in MediaView.tsx line 69), but you should verify that the modal closes correctly when:
- User clicks the backdrop
- User presses Escape key
- User clicks the close button
Test the MediaView modal behavior on the Favourites page to confirm it closes properly without an explicit close handler.
frontend/src/components/Media/MediaView.tsx (1)
80-85: Good integration with useToggleFav hook.The favorite toggle logic correctly uses the
useToggleFavhook, which handles the backend API call and cache invalidation. This is a proper implementation following the project's patterns for mutations.However, ensure you address the state synchronization issues flagged in other comments so the UI properly reflects the favorite status after navigation.
| const title = | ||
| isSearchActive && images.length > 0 | ||
| ? `Face Search Results (${images.length} found)` | ||
| : 'Favourite Image Gallery'; |
There was a problem hiding this comment.
Questionable: Search state logic on Favourites page.
The title computation checks isSearchActive and shows "Face Search Results" when search is active. This seems out of place on a dedicated Favourites page—users navigating to "Favourites" would not expect to see search results here.
Consider one of these approaches:
- Remove search integration entirely (recommended for Favourites page):
const title =
- isSearchActive && images.length > 0
- ? `Face Search Results (${images.length} found)`
- : 'Favourite Image Gallery';
+ `Favourite Image Gallery (${favouriteImages.length})`;- Or clarify the intent: If search results should affect this page, the filtering logic needs to account for both search results AND favorites (currently it only filters by
isFavourite).
📝 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 title = | |
| isSearchActive && images.length > 0 | |
| ? `Face Search Results (${images.length} found)` | |
| : 'Favourite Image Gallery'; | |
| const title = | |
| `Favourite Image Gallery (${favouriteImages.length})`; |
🤖 Prompt for AI Agents
In frontend/src/pages/Home/MyFav.tsx around lines 66 to 69, the title uses
isSearchActive to switch to "Face Search Results" which is inappropriate for the
dedicated Favourites page; either remove the search integration and always set
the title to "Favourite Image Gallery", or if search must apply, update the
filtering to combine the search results with isFavourite (filter images by both
the search predicate and isFavourite) and change the title to reflect scoped
results (e.g. "Face Search Results (N found in favourites)") so the UI and data
filtering stay consistent.
| {favouriteImages.length > 0 ? ( | ||
| <ChronologicalGallery | ||
| images={favouriteImages} | ||
| showTitle={true} | ||
| title={title} | ||
| onMonthOffsetsChange={setMonthMarkers} | ||
| scrollContainerRef={scrollableRef} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify index mapping between filtered array and MediaView.
ChronologicalGallery receives the filtered favouriteImages array and will render ImageCard components. When a card is clicked, based on the ChronologicalGallery implementation, it dispatches setCurrentViewIndex with a chronological index computed from the full image set via imageIndexMap.
However, you should verify that:
- The
imageIndexMapin ChronologicalGallery correctly maps filtered favourite images back to their global indices - MediaView displays the correct image when opened from this page
🏁 Script executed:
#!/bin/bash
# Verify ChronologicalGallery's imageIndexMap implementation
ast-grep --pattern $'const imageIndexMap = $$$
$$$
imageIndexMap[$_] = $_
$$$'
# Check how ImageCard dispatches index on click
ast-grep --pattern $'onClick={() => {
$$$
dispatch(setCurrentViewIndex($_))
$$$
}}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find ChronologicalGallery component file
fd -e tsx -e ts | xargs rg -l "ChronologicalGallery" | head -10
# Search for imageIndexMap usage
rg -n "imageIndexMap" --type=tsx --type=ts -C 3
# Search for setCurrentViewIndex dispatch patterns
rg -nP "setCurrentViewIndex|dispatch.*CurrentViewIndex" --type=tsx --type=ts -C 2Length of output: 740
🏁 Script executed:
#!/bin/bash
# Locate ImageCard component and its onClick handler
fd -e tsx -e ts | xargs rg -l "ImageCard" | head -10
# Search for onClick handlers that dispatch index changes
rg -nP "onClick.*=>\s*\{" --type=tsx --type=ts -A 5 | head -50Length of output: 604
🏁 Script executed:
#!/bin/bash
# Check MyFav.tsx more thoroughly to see how ChronologicalGallery is used
rg -n "ChronologicalGallery\|favouriteImages\|scrollableRef" frontend/src/pages/Home/MyFav.tsx -B 2 -A 5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Examine ChronologicalGallery component for imageIndexMap
cat frontend/src/components/Media/ChronologicalGallery.tsxLength of output: 5838
🏁 Script executed:
#!/bin/bash
# Examine ImageCard component for onClick handler
cat frontend/src/components/Media/ImageCard.tsxLength of output: 3667
🏁 Script executed:
#!/bin/bash
# Check MyFav.tsx for complete context
cat frontend/src/pages/Home/MyFav.tsxLength of output: 4244
🏁 Script executed:
#!/bin/bash
# Examine MediaView to see how it uses currentViewIndex
cat frontend/src/components/Media/MediaView.tsxLength of output: 6804
🏁 Script executed:
#!/bin/bash
# Search for setCurrentViewIndex usage to understand what index it expects
rg -n "setCurrentViewIndex\|currentViewIndex" frontend/src/features/imageSlice.ts -B 2 -A 5Length of output: 0
Fix index mapping to use global image IDs instead of filtered gallery positions.
When ImageCard is clicked in the filtered favourite gallery, imageIndexMap provides an index relative to the filtered favouriteImages array (lines 196-198 of ChronologicalGallery.tsx). However, MediaView uses currentViewIndex directly on the global images array (line 40 of MediaView.tsx), causing the wrong image to display.
For example: if global images are [img1, img2, img3, img4, img5] and filtered favourites are [img2, img4], clicking img2 would dispatch index 0 but MediaView would show images[0] = img1.
Resolution: Pass the global image ID (or global index) to setCurrentViewIndex, then resolve it in MediaView using the image's ID or by finding its position in the global array. Alternatively, modify imageIndexMap to map to global indices instead of filtered positions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
frontend/src/pages/Home/MyFav.tsx (2)
55-57: Remove commented-out code.The commented
handleCloseMediaViewfunction is unused since MediaView is not rendered in this component. Remove this dead code as requested by the maintainer.Apply this diff:
- // const handleCloseMediaView = () => { - // // MediaView will handle closing via Redux - // }; -
64-67: Remove search state logic from Favourites page.The title computation showing "Face Search Results" is inappropriate for a dedicated Favourites page. Users navigating to "Favourites" expect to see their favourite images, not search results.
Apply this diff:
- const title = - isSearchActive && images.length > 0 - ? `Face Search Results (${images.length} found)` - : 'Favourite Image Gallery'; + const title = `Favourite Image Gallery (${favouriteImages.length})`;frontend/src/components/Media/MediaView.tsx (6)
3-3: Remove commented-out import.The commented import should be removed as requested by the maintainer to clean up unnecessary comments.
Apply this diff:
-// import { revealItemInDir } from '@tauri-apps/plugin-opener';
34-36: Remove unnecessary comments.As requested by the maintainer, remove these debug/guard explanation comments.
Apply this diff:
- // guard: images default to empty array in the signature so `images.length` is safe - // keep debug output minimal - // console.log(totalImages);
100-100: Remove commented-out code.The commented
revealItemInDircall should be removed per maintainer's request.Apply this diff:
- // await revealItemInDir(currentImage.path);
152-152: Remove commented console.log.Remove this debug statement as requested by the maintainer.
Apply this diff:
- // console.log(currentImage);
55-55: Syncisfavstate whencurrentImagechanges.The
isfavstate is initialized once but doesn't update when the user navigates to a different image, causing the favorite icon to show stale state.Apply this diff:
+import { useEffect } from 'react'; + const [isfav, setIsfav] = useState(currentImage?.isFavourite || false); + +useEffect(() => { + setIsfav(currentImage?.isFavourite || false); +}, [currentImage?.isFavourite]);
113-118: Remove redundant local state toggle.The
handleToggleFavoritefunction redundantly togglesisfavbefore callinghandle_favourite_toggle. Since the favourite mutation already handles state updates and cache invalidation, this creates unnecessary state churn. The proper state sync should come from theuseEffectthat watchescurrentImage?.isFavourite(once added per previous comment).Apply this diff:
const handleToggleFavorite = useCallback(() => { if (currentImage) { - setIsfav((prev) => !prev); handle_favourite_toggle(); } - }, [currentImage, isfav]); + }, [currentImage, handle_favourite_toggle]);
🧹 Nitpick comments (1)
frontend/src/pages/Home/MyFav.tsx (1)
11-11: Minor: Remove trailing space in import.There's an extra space after
selectImages}in the import statement.Apply this diff:
-import { selectImages} from '@/features/imageSelectors'; +import { selectImages } from '@/features/imageSelectors';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/Media/MediaView.tsx(7 hunks)frontend/src/pages/Home/MyFav.tsx(1 hunks)frontend/src/types/Media.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/types/Media.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/pages/Home/MyFav.tsx (11)
frontend/src/features/imageSelectors.ts (1)
selectImages(5-7)frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(10-14)ChronologicalGallery(25-187)frontend/src/app/store.ts (1)
RootState(22-22)frontend/src/hooks/useQueryExtension.ts (1)
usePictoQuery(80-108)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(5-14)frontend/src/features/loaderSlice.ts (2)
showLoader(17-20)hideLoader(21-24)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)frontend/src/types/Media.ts (1)
Image(13-23)frontend/src/features/imageSlice.ts (1)
setImages(18-20)frontend/src/components/EmptyStates/EmptyGalleryState.tsx (1)
EmptyGalleryState(3-28)frontend/src/components/Timeline/TimelineScrollbar.tsx (1)
TimelineScrollbar(38-409)
frontend/src/components/Media/MediaView.tsx (5)
frontend/src/types/Media.ts (1)
MediaViewProps(35-39)frontend/src/features/imageSelectors.ts (1)
selectCurrentViewIndex(9-10)frontend/src/hooks/useFavorites.ts (1)
useFavorites(3-28)frontend/src/hooks/useToggleFav.ts (1)
useToggleFav(5-17)frontend/src/hooks/useSlideshow.ts (1)
useSlideshow(3-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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 (2)
frontend/src/pages/Home/MyFav.tsx (2)
69-90: LGTM: Well-implemented empty state.The empty state provides clear messaging and uses the Heart icon from lucide-react as requested by the maintainer. The UX guidance ("Click the heart icon on any image") is helpful.
99-109: Remove redundant conditional check.The check
favouriteImages.length > 0at line 99 is redundant because the component already returns early at line 69 iffavouriteImages.length === 0. This means the else branch (lines 107-109) renderingEmptyGalleryStateis unreachable.Apply this diff:
- {favouriteImages.length > 0 ? ( - <ChronologicalGallery - images={favouriteImages} - showTitle={true} - title={title} - onMonthOffsetsChange={setMonthMarkers} - scrollContainerRef={scrollableRef} - /> - ) : ( - <EmptyGalleryState /> - )} + <ChronologicalGallery + images={favouriteImages} + showTitle={true} + title={title} + onMonthOffsetsChange={setMonthMarkers} + scrollContainerRef={scrollableRef} + />Likely an incorrect or invalid review comment.
|
@rahulharpal1603 please review sir |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/components/Media/ImageCard.tsx (1)
28-28: Remove redundant local state.The
isfavstate is never synchronized withimage.isFavourite(initialized tofalseregardless of actual value), is not used anywhere in the UI (lines 83, 93 read fromimage.isFavouriteinstead), and serves no purpose. TheuseToggleFavhook already auto-invalidates the'images'cache, soimage.isFavouritewill update automatically after the mutation completes.Apply this diff to remove the redundant state:
- const [isfav, setIsfav] = useState(false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Media/ImageCard.tsx(3 hunks)frontend/src/pages/Home/MyFav.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/Home/MyFav.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/Media/ImageCard.tsx (1)
frontend/src/hooks/useToggleFav.ts (1)
useToggleFav(5-17)
⏰ 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 (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Backend Tests
🔇 Additional comments (1)
frontend/src/components/Media/ImageCard.tsx (1)
82-97: UI logic correctly reflects favorite state from props.The button styling and icon rendering properly use
image.isFavouritefrom props. Once the redundant local state is removed and the mutation hook completes, the cache invalidation will automatically updateimage.isFavourite, providing a clean unidirectional data flow.
| const handleToggleFavorite = useCallback(() => { | ||
| if (image) { | ||
| setIsfav((prev) => !prev); | ||
| handle_favourite_toggle(); | ||
| } | ||
| }, [image, isfav]); |
There was a problem hiding this comment.
Remove unnecessary wrapper and fix useCallback dependencies.
Two issues:
- Incorrect
useCallbackdependency: Includingisfavin the dependency array causes the callback to be recreated on every toggle, defeating the purpose of memoization. - Unnecessary wrapper: This function simply toggles redundant local state and calls
handle_favourite_toggle. Once the local state is removed (per previous comment), this wrapper adds no value.
Apply this diff after removing the isfav state:
- const handleToggleFavorite = useCallback(() => {
- if (image) {
- setIsfav((prev) => !prev);
- handle_favourite_toggle();
- }
- }, [image, isfav]);Then update line 90 to call handle_favourite_toggle directly:
- handleToggleFavorite();
+ handle_favourite_toggle();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/Media/ImageCard.tsx around lines 38 to 43, the
handleToggleFavorite wrapper is unnecessary and its useCallback dependency
incorrectly includes isfav; after removing the local isfav state, delete this
handleToggleFavorite function (and its useCallback) and update the call site at
line 90 to invoke handle_favourite_toggle() directly (no wrapper, no extra
deps).
| : 'bg-white/10 hover:bg-white/20 hover:shadow-lg' | ||
| }`} | ||
| onClick={(e) => { | ||
| console.log(image); |
There was a problem hiding this comment.
Remove debugging artifact.
The console.log(image) statement should be removed before merging.
Apply this diff:
onClick={(e) => {
- console.log(image);
e.stopPropagation();
handleToggleFavorite();
}}📝 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.
| console.log(image); | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| handleToggleFavorite(); | |
| }} |
🤖 Prompt for AI Agents
In frontend/src/components/Media/ImageCard.tsx around line 88, remove the
debugging artifact by deleting the console.log(image) statement; ensure no other
code depends on that log and that formatting/linting remain clean after removal.
rahulharpal1603
left a comment
rahulharpal1603
left a comment
There was a problem hiding this comment.
Thanks @Vaibhaviitian
I have made some changes. Look at this commit: 09ab8be
This will help you in future to write an even better code.
|
@rahulharpal1603 thanks sir, definitely i will read this |
|
@rahulharpal1603 wait sir i am resolving conflicts |
|
@rahulharpal1603 sir just one doubt as that one is not made by me so someone might think that this one is not mine contribution even though my commits are there |
No it's not like that. I had to squash and merge the PR because there were 40 commits and I wanted to keep the commit history clean. I always know that it was your PR :) And I am assigning you that PR so that anyone who opens it up will know that you did the major work. |
Comment below that PR so I can assign it to you. |
|
Sir which pr #634 or i have to create new one |
comment on 634 only |
|
okay sir |
|
This pr was |
2 participants
PR: Fix #538 – Add “Favorites” feature
What this PR does:
Users can now add images to favorites with a single click.
Created a Favorites page to view and manage all favorite images.
All changes are synced with the database.
Added a simple UI for easy management.
Screen.Recording.2025-11-01.050141.1.mp4
Summary by CodeRabbit
New Features
Documentation