Fix Issues in favourites PR #634
Conversation
…sed favorites hook, and enhance input schema in OpenAPI spec
|
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds server-side favourite support: a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImageCard
participant useToggleFav
participant API as Backend API
participant DB as Database
User->>ImageCard: Click Favourite Button
ImageCard->>useToggleFav: toggleFavourite(image_id)
useToggleFav->>API: POST /images/toggle-favourite
API->>DB: Toggle isFavourite flag
DB-->>API: Updated status
API-->>useToggleFav: Return status
useToggleFav->>ImageCard: Invalidate/re-query images
ImageCard->>User: UI updates to new isFavourite
sequenceDiagram
participant User
participant Sidebar
participant MyFav
participant API
User->>Sidebar: Click "Favourites"
Sidebar->>MyFav: Navigate to /favourites
MyFav->>API: Fetch images (usePictoQuery)
API-->>MyFav: Return images with isFavourite
MyFav->>MyFav: Filter images where isFavourite === true
alt Favourite images exist
MyFav->>User: Render ChronologicalGallery
else no favourites
MyFav->>User: Show empty state message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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 |
|
|
2 similar comments
|
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
backend/app/database/images.py (1)
52-70: Add schema migration for isFavourite column to handle existing databasesThe review comment correctly identifies a backward compatibility issue. The code adds
isFavourite BOOLEAN DEFAULT 0to theCREATE TABLE IF NOT EXISTS imagesstatement (line 66), and then references this column indb_get_all_images(line 147) anddb_toggle_image_favourite_status(line 409). However, codebase search confirms there is no ALTER TABLE, schema checking (PRAGMA), or migration logic anywhere to add this column to existing databases. Deployments with pre-existingimagestables will fail on queries referencingisFavourite.Add a migration function (e.g.,
db_ensure_is_favourite_column()) that runs during startup to check if the column exists and add it if missing, as suggested in the review comment. Alternatively, ensure your deployment process rebuilds/resets the database schema.Also note:
db_toggle_image_favourite_status(line 399) should use_connect()instead ofsqlite3.connect(DATABASE_PATH)directly for consistency with other functions in this module.
🧹 Nitpick comments (11)
frontend/src/pages/Home/Home.tsx (1)
25-46: Consider usingerrorMessagefromusePictoQueryand guarding against missing dataTwo small, optional improvements here:
usePictoQueryalready derives a user-friendlyerrorMessage. You could destructure it and pass it through touseMutationFeedbackfor richer error feedback instead of a static string, e.g.:- const { data, isLoading, isSuccess, isError, error } = usePictoQuery({ + const { + data, + isLoading, + isSuccess, + isError, + error, + errorMessage, + } = usePictoQuery({ queryKey: ['images'], queryFn: () => fetchAllImages(), enabled: !isSearchActive, }); - useMutationFeedback( - { isPending: isLoading, isSuccess, isError, error }, - { - loadingMessage: 'Loading images', - showSuccess: false, - errorTitle: 'Error', - errorMessage: 'Failed to load images. Please try again later.', - }, - ); + useMutationFeedback( + { isPending: isLoading, isSuccess, isError, error }, + { + loadingMessage: 'Loading images', + showSuccess: false, + errorTitle: 'Error', + errorMessage: errorMessage ?? 'Failed to load images. Please try again later.', + }, + );
- In the effect,
isSuccessshould imply data is present, but defensively defaulting to an empty array avoids surprises if that ever changes:- if (!isSearchActive && isSuccess) { - const images = data?.data as Image[]; - dispatch(setImages(images)); - } + if (!isSearchActive && isSuccess) { + const images = (data?.data as Image[]) ?? []; + dispatch(setImages(images)); + }Both are optional, but they make the behavior a bit more robust and user-friendly.
frontend/src/api/apiEndpoints.ts (1)
1-4: Endpoint naming vs behaviour (optional polish)The new
setFavourite: '/images/toggle-favourite'endpoint looks correct, but the name suggests “set” while the backend route semantically “toggles” the flag. Consider renaming the key totoggleFavouritefor clarity if it won’t cause churn.frontend/src/api/api-functions/togglefav.ts (1)
1-11: togglefav helper is correct; consider naming consistencyThe POST call, typing, and payload shape all look good. For consistency with the rest of the codebase (
useToggleFav,isFavourite), consider renaming this function totoggleFavourite(and adjusting imports) so the naming is uniform.frontend/src/hooks/useToggleFav.ts (1)
5-17: Tighten toggleFavourite parameter typingImplementation and invalidation logic look good, but
toggleFavourite: (id: any)loses type safety even though the mutation clearly expects a string ID.You can strengthen this by typing the variable and mutation generics:
-export const useToggleFav = () => { - const toggleFavouriteMutation = usePictoMutation({ - mutationFn: async (image_id: string) => togglefav(image_id), +export const useToggleFav = () => { + const toggleFavouriteMutation = usePictoMutation<APIResponse, unknown, string>({ + mutationFn: async (image_id: string) => togglefav(image_id), autoInvalidateTags: ['images'], }); … return { - toggleFavourite: (id: any) => toggleFavouriteMutation.mutate(id), + toggleFavourite: (id: string) => toggleFavouriteMutation.mutate(id), toggleFavouritePending: toggleFavouriteMutation.isPending, }; };(or similar, depending on your existing
usePictoMutationtypings).backend/app/database/images.py (2)
167-191: isFavourite mapping in db_get_all_images is fine; align types for clarityThe new unpacked
is_favouritefield andbool(is_favourite)mapping into"isFavourite"in the image dict are correct and match frontend expectations. To keep your schema and types in sync, consider addingisFavourite: boolto theImageRecordTypedDict(and any related record types) so future inserts and tooling reflect the full table structure.
399-421: Reuse _connect and type aliases in db_toggle_image_favourite_statusFunctionality is correct, but this helper is slightly inconsistent with the rest of the module:
- It calls
sqlite3.connect(DATABASE_PATH)directly instead of using_connect(), so it bypasses the shared connection setup (including thePRAGMA foreign_keys = ON).- It takes
image_id: strinstead of the existingImageIdalias.You can align it with the rest of the file like this:
-def db_toggle_image_favourite_status(image_id: str) -> bool: - conn = sqlite3.connect(DATABASE_PATH) +def db_toggle_image_favourite_status(image_id: ImageId) -> bool: + conn = _connect() cursor = conn.cursor()The rest of the function can stay as‑is.
frontend/src/components/Media/ImageCard.tsx (1)
30-36: Tidy up favourite handler (remove debug log, optional UX polish).The toggle wiring via
useToggleFavandhandleToggleFavouritelooks good, but:
- Line 81’s
console.log(image);should be removed before shipping.- You may also want to use
toggleFavouritePendingfromuseToggleFavto disable the button while the mutation is in flight to avoid rapid repeat toggles (optional).Also applies to: 75-84
docs/backend/backend_python/openapi.json (1)
890-928: Align toggle-favourite OpenAPI response with backend payload.The additions of
ImageData.isFavouriteandToggleFavouriteRequestlook consistent with the backend models. However, the/images/toggle-favourite200 response currently uses an empty schema ({}), while the route returns a structured object (success,image_id,isFavourite). It would be better to define and reference a concrete response schema (e.g.ToggleFavouriteResponseor reuseImageData/ImageInfoResponse) so clients get accurate typing from the OpenAPI spec.Also applies to: 2111-2164, 2553-2565
backend/app/routes/images.py (1)
122-130: Remove or wire up unused ImageInfoResponse model.
ImageInfoResponseis defined but not referenced as aresponse_modelor used elsewhere. Either hook it up (e.g. as the response model for/images/toggle-favourite) or remove it to avoid dead code.frontend/src/pages/Home/MyFav.tsx (1)
50-59: Clarify title/count semantics for favourites view.
titleusesimages.lengthto showFace Search Results (N found)when a search is active, but the page actually renders onlyfavouriteImages(filtered byisFavourite === true). This can make the count misleading when there are search hits that are not favourites.Consider either:
- Basing the count on
favouriteImages.lengthwhen on this favourites page, or- Changing the title text to explicitly indicate you’re viewing the favourite subset of the search results.
Also, because you return early when
favouriteImages.length === 0, theEmptyGalleryStatebranch inside the gallery (lines 90-100) is unreachable and can be removed for clarity.Also applies to: 60-81
frontend/src/components/Media/MediaView.tsx (1)
24-35: Defaulting images to an empty array plus guard is reasonable.Providing a default
images = []and guarding withif (!images?.length || currentViewIndex === -1 || !currentImage)prevents the viewer from rendering in invalid states and avoids length access onundefined. Just note thatMediaViewProps.imagesis still non-optional; if you intend to truly make it optional, consider updating the type as well.Also applies to: 140-143
📜 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 (20)
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(4 hunks)frontend/scripts/setup_env.sh(1 hunks)frontend/scripts/setup_win.ps1(1 hunks)frontend/src/api/api-functions/togglefav.ts(1 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/components/Media/ImageCard.tsx(3 hunks)frontend/src/components/Media/MediaThumbnails.tsx(0 hunks)frontend/src/components/Media/MediaView.tsx(5 hunks)frontend/src/components/Media/MediaViewControls.tsx(3 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/hooks/useToggleFav.ts(1 hunks)frontend/src/pages/Home/Home.tsx(2 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/components/Media/MediaThumbnails.tsx
🧰 Additional context used
🧬 Code graph analysis (9)
frontend/src/api/api-functions/togglefav.ts (3)
frontend/src/types/API.ts (1)
APIResponse(1-8)frontend/src/api/axiosConfig.ts (1)
apiClient(5-12)frontend/src/api/apiEndpoints.ts (1)
imagesEndpoints(1-4)
frontend/src/pages/Home/MyFav.tsx (10)
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/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)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/ImageCard.tsx (1)
frontend/src/hooks/useToggleFav.ts (1)
useToggleFav(5-18)
frontend/src/routes/AppRoutes.tsx (2)
frontend/src/constants/routes.ts (1)
ROUTES(1-12)frontend/src/pages/Home/MyFav.tsx (1)
MyFav(18-115)
frontend/src/components/Navigation/Sidebar/AppSidebar.tsx (1)
frontend/src/constants/routes.ts (1)
ROUTES(1-12)
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)
frontend/src/hooks/useToggleFav.ts (3)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/togglefav.ts (1)
togglefav(5-11)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)
frontend/src/pages/Home/Home.tsx (5)
frontend/src/hooks/useQueryExtension.ts (1)
usePictoQuery(80-108)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(5-14)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)frontend/src/types/Media.ts (1)
Image(13-23)frontend/src/features/imageSlice.ts (1)
setImages(18-20)
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/useToggleFav.ts (1)
useToggleFav(5-18)frontend/src/hooks/useSlideshow.ts (1)
useSlideshow(3-30)frontend/src/components/Media/MediaViewControls.tsx (1)
MediaViewControls(16-100)
⏰ 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). (5)
- GitHub Check: sync-labels
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
🔇 Additional comments (10)
frontend/scripts/setup_win.ps1 (1)
73-73: Non-functional EOF formatting change is fineOnly the trailing newline/EOF formatting changed here; the message and behavior remain the same and look good.
frontend/scripts/setup_env.sh (1)
118-118: EOF newline addition only; behavior unchangedThis change just ensures the file ends with a newline; the note about restarting the terminal remains correct and helpful.
frontend/src/pages/Home/Home.tsx (1)
15-39: Centralizing loader/error handling withuseMutationFeedbacklooks solidMapping the query state into
useMutationFeedback(isPending: isLoading, isSuccess, isError, error) and disabling success to avoid a dialog on every successful fetch is a clean way to reuse the global loader/error UI without duplicating logic.frontend/src/types/Media.ts (1)
13-23:isFavouritefield addition is consistent and non-breakingAdding
isFavourite?: booleantoImagecleanly models the new favourites feature while keeping the property optional so existing usages ofImageremain valid.frontend/src/components/Navigation/Sidebar/AppSidebar.tsx (1)
15-47: Favourites sidebar entry is wired correctlyImporting
Heartand adding theFavouritesmenu item pointing to/${ROUTES.FAVOURITES}integrates cleanly with the existing menu structure and active-route logic.frontend/src/constants/routes.ts (1)
3-3: New FAVOURITES route is consistent with existing patternsKey name and path segment match the existing routing style; no issues found.
frontend/src/routes/AppRoutes.tsx (1)
8-8: MyFav route wiring under /favourites looks correctImporting
MyFavand mounting it atROUTES.FAVOURITESinside theLayoutmatches how the other main pages are registered; routing setup looks good.Also applies to: 20-20
backend/app/routes/images.py (1)
29-38: isFavourite field wiring in ImageData looks consistent.Adding
isFavourite: booltoImageDataand mapping it viaimage.get('isFavourite', False)inget_all_imageskeeps the API strongly typed while remaining backwards compatible with older rows that may lack the column.Also applies to: 60-69
frontend/src/pages/Home/MyFav.tsx (1)
26-48: Data fetching and feedback wiring look solid.Using
usePictoQuerywithenabled: !isSearchActiveand feedingisLoading/isSuccessintouseMutationFeedbackis a clean way to reuse your feedback hook for queries. ThesetImagesdispatch on successful fetch is straightforward.frontend/src/components/Media/MediaViewControls.tsx (1)
4-14: LGTM! Prop renames verified across all call sites.The prop name updates from American to British spelling (
onToggleFavorite→onToggleFavourite,isFavorite→isFavourite) have been consistently applied throughout the codebase. Verification confirms:
- MediaViewControls.tsx component definition uses new prop names
- MediaView.tsx (parent component) has been updated to pass the new prop names
- No remaining references to old prop names exist in the codebase
|
|
|
|
|
|
|
@rahulharpal1603 sir as the contri in this pr is major mine so can u please assign this to me |
Done. |
|
Thank you, Sir. |
2 participants
Fixes issues in PR #551
Major changes done by @Vaibhaviitian.
Summary by CodeRabbit