feat: implement albums feature with create, edit, delete, and image management UI #610
Conversation
WalkthroughReplaces album visibility field Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Albums UI
participant API as Backend API
participant DB as Database
User->>UI: Open CreateAlbumDialog
UI->>User: Fill form (name, is_locked, password)
User->>UI: Submit
UI->>API: POST /albums {name,is_locked,password}
API->>DB: db_insert_album(...)
DB-->>API: OK
API-->>UI: 201 Created
UI->>API: GET /albums
API->>DB: db_get_all_albums()
DB-->>API: [albums with cover_image_path,image_count]
API-->>UI: Album list
sequenceDiagram
participant User
participant Detail as AlbumDetail Page
participant AddDialog as AddImagesToAlbumDialog
participant API as Backend API
participant DB as Database
User->>Detail: View album
Detail->>API: GET /albums/{id}
API->>DB: db_get_album(id)
DB-->>API: album (cover_image_path,image_count)
API-->>Detail: album data
User->>Detail: Add Images
Detail->>AddDialog: open
AddDialog->>API: GET /images
API->>DB: db_get_all_images()
DB-->>API: [images]
API-->>AddDialog: images list
AddDialog->>API: POST /albums/{id}/images {image_ids}
API->>DB: associate images
DB-->>API: OK
API-->>Detail: trigger refresh -> GET /albums/{id}/images
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SiddharthJiyani
force-pushed
the
feat/albums-ui-and-backend
branch
from
ff90128 to
afcecea
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/types/Album.ts (1)
59-92: Remove duplicate interface definitions.
PasswordPromptDialogPropsis defined twice (lines 59-64 and 80-85), andAddImagesToAlbumDialogPropsis also defined twice (lines 66-71 and 87-92). This creates confusion and potential maintenance issues if the definitions diverge.Remove the duplicate definitions:
export interface CreateAlbumDialogProps { isOpen: boolean; onClose: () => void; onSuccess?: () => void; } export interface PasswordPromptDialogProps { isOpen: boolean; onClose: () => void; onSubmit: (password: string) => void; albumName: string; } +export interface AddImagesToAlbumDialogProps { + isOpen: boolean; + onClose: () => void; + albumId: string; + albumName: string; +} + export interface AlbumCardProps { album: Album; onClick: () => void; onEdit: () => void; onDelete: () => void; } - -export interface PasswordPromptDialogProps { - isOpen: boolean; - onClose: () => void; - onSubmit: (password: string) => void; - albumName: string; -} - -export interface AddImagesToAlbumDialogProps { - isOpen: boolean; - onClose: () => void; - albumId: string; - albumName: string; -}
🧹 Nitpick comments (4)
frontend/src/components/EmptyStates/EmptyAlbumsState.tsx (1)
3-28: Consider addingaria-hiddento decorative icons.The component structure and implementation are excellent. The layout, styling, dark mode support, and user guidance are well-executed.
For enhanced accessibility, consider adding
aria-hidden="true"to decorative icons on lines 7, 18, and 22, since they don't convey information beyond what the text already provides.Example for line 7:
- <BookImage className="h-16 w-16 text-gray-400" strokeWidth={1.5} /> + <BookImage className="h-16 w-16 text-gray-400" strokeWidth={1.5} aria-hidden="true" />frontend/src/pages/Album/AlbumDetail.tsx (2)
153-174: Backend data transformation handles schema variations defensively.The transformation logic safely handles multiple possible backend response structures using optional chaining and defaults. However, the use of
anytypes suggests the backend API contract could benefit from stronger typing.Consider defining a TypeScript interface for the backend album response to eliminate the
anycasts and improve type safety:interface BackendAlbum { album_id: string; album_name: string; description?: string; is_locked?: boolean; cover_image_path?: string; image_count?: number; created_at?: string; updated_at?: string; }
176-213: Image loading works but has inefficient data fetching pattern.The implementation fetches all images from the library, then filters by album image IDs. While functional, this becomes inefficient as the image library grows.
Consider updating the backend
getAlbumImagesendpoint to return full image objects rather than just IDs, eliminating the need to fetch and filter the entire image library:// Instead of fetching all images const { data: allImagesData } = usePictoQuery({ queryKey: ['images'], queryFn: () => fetchAllImages(), enabled: !!albumId && !!album, }); // Backend could return full image objects directly const { data: imagesData } = usePictoQuery({ queryKey: ['album-images', albumId], queryFn: () => getAlbumImages(albumId!, password ? { password } : undefined), enabled: !!albumId && !!album, });frontend/src/features/albumsSlice.ts (1)
56-70: Consider syncing image_count from server.The local
image_countupdates (increment on add, decrement on remove) are correct, but could drift from the server if operations fail or are retried. Consider refreshing the selected album from the server after image operations complete to ensure consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/public/placeholder-album-light.svgis excluded by!**/*.svgfrontend/public/placeholder-album.svgis excluded by!**/*.svg
📒 Files selected for processing (21)
backend/app/database/albums.py(10 hunks)backend/app/routes/albums.py(10 hunks)backend/app/schemas/album.py(3 hunks)frontend/src/api/api-functions/albums.ts(1 hunks)frontend/src/api/api-functions/index.ts(1 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/app/store.ts(2 hunks)frontend/src/components/Albums/AddImagesToAlbumDialog.tsx(1 hunks)frontend/src/components/Albums/AlbumCard.tsx(1 hunks)frontend/src/components/Albums/AlbumPasswordDialog.tsx(1 hunks)frontend/src/components/Albums/CreateAlbumDialog.tsx(1 hunks)frontend/src/components/Albums/DeleteConfirmDialog.tsx(1 hunks)frontend/src/components/Albums/EditAlbumDialog.tsx(1 hunks)frontend/src/components/EmptyStates/EmptyAlbumsState.tsx(1 hunks)frontend/src/constants/routes.ts(1 hunks)frontend/src/features/albumSelectors.ts(1 hunks)frontend/src/features/albumsSlice.ts(1 hunks)frontend/src/pages/Album/Album.tsx(1 hunks)frontend/src/pages/Album/AlbumDetail.tsx(1 hunks)frontend/src/routes/AppRoutes.tsx(2 hunks)frontend/src/types/Album.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
frontend/src/components/Albums/AlbumCard.tsx (2)
frontend/src/types/Album.ts (1)
AlbumCardProps(73-78)frontend/src/contexts/ThemeContext.tsx (1)
useTheme(66-73)
frontend/src/components/Albums/CreateAlbumDialog.tsx (5)
frontend/src/types/Album.ts (1)
CreateAlbumDialogProps(53-57)frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/albums.ts (1)
createAlbum(37-45)frontend/src/features/loaderSlice.ts (2)
hideLoader(21-24)showLoader(17-20)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)
frontend/src/components/Albums/AlbumPasswordDialog.tsx (1)
frontend/src/types/Album.ts (2)
PasswordPromptDialogProps(59-64)PasswordPromptDialogProps(80-85)
backend/app/schemas/album.py (1)
frontend/src/types/Album.ts (1)
UpdateAlbumRequest(26-32)
frontend/src/features/albumsSlice.ts (2)
frontend/src/types/Album.ts (1)
Album(1-10)frontend/src/types/Media.ts (1)
Image(13-22)
frontend/src/components/Albums/EditAlbumDialog.tsx (5)
frontend/src/types/Album.ts (1)
EditAlbumDialogProps(46-51)frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/albums.ts (1)
updateAlbum(52-61)frontend/src/features/loaderSlice.ts (2)
hideLoader(21-24)showLoader(17-20)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)
frontend/src/api/api-functions/albums.ts (4)
frontend/src/types/API.ts (1)
APIResponse(1-8)frontend/src/api/axiosConfig.ts (1)
apiClient(5-12)frontend/src/api/apiEndpoints.ts (1)
albumsEndpoints(33-46)frontend/src/types/Album.ts (5)
CreateAlbumRequest(19-24)UpdateAlbumRequest(26-32)AddImagesToAlbumRequest(34-36)GetAlbumImagesRequest(38-40)RemoveImagesFromAlbumRequest(42-44)
backend/app/routes/albums.py (2)
backend/app/schemas/album.py (5)
SetCoverImageRequest(55-56)GetAlbumsResponse(64-66)Album(6-12)SuccessResponse(84-86)ErrorResponse(89-92)backend/app/database/albums.py (5)
db_update_album_cover_image(221-232)verify_album_password(299-311)db_get_all_albums(107-119)db_get_album_images(235-245)db_get_album(136-147)
frontend/src/routes/AppRoutes.tsx (2)
frontend/src/constants/routes.ts (1)
ROUTES(1-12)frontend/src/pages/Album/AlbumDetail.tsx (1)
AlbumDetail(43-437)
frontend/src/components/Albums/AddImagesToAlbumDialog.tsx (7)
frontend/src/types/Album.ts (2)
AddImagesToAlbumDialogProps(66-71)AddImagesToAlbumDialogProps(87-92)frontend/src/types/Media.ts (1)
Image(13-22)frontend/src/hooks/useQueryExtension.ts (2)
usePictoQuery(80-108)usePictoMutation(26-78)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(5-14)frontend/src/api/api-functions/albums.ts (1)
addImagesToAlbum(79-88)frontend/src/features/loaderSlice.ts (2)
hideLoader(21-24)showLoader(17-20)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)
frontend/src/components/Albums/DeleteConfirmDialog.tsx (1)
frontend/src/components/ui/dialog.tsx (6)
Dialog(133-133)DialogContent(135-135)DialogHeader(138-138)DialogTitle(141-141)DialogDescription(136-136)DialogFooter(137-137)
frontend/src/features/albumSelectors.ts (1)
frontend/src/app/store.ts (1)
RootState(24-24)
frontend/src/types/Album.ts (1)
backend/app/schemas/album.py (4)
CreateAlbumRequest(20-30)UpdateAlbumRequest(33-44)GetAlbumImagesRequest(47-48)Album(6-12)
frontend/src/pages/Album/AlbumDetail.tsx (13)
frontend/src/features/albumSelectors.ts (2)
selectSelectedAlbum(7-8)selectAlbumImages(10-10)frontend/src/features/imageSelectors.ts (1)
selectIsImageViewOpen(13-16)frontend/src/hooks/useQueryExtension.ts (2)
usePictoQuery(80-108)usePictoMutation(26-78)frontend/src/api/api-functions/albums.ts (4)
getAlbumById(26-31)getAlbumImages(95-104)removeMultipleImagesFromAlbum(126-135)setAlbumCoverImage(142-151)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(5-14)frontend/src/features/loaderSlice.ts (2)
hideLoader(21-24)showLoader(17-20)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)frontend/src/types/Album.ts (1)
Album(1-10)frontend/src/features/albumsSlice.ts (3)
setSelectedAlbum(48-50)setAlbumImages(52-54)clearAlbumImages(72-74)frontend/src/features/imageSlice.ts (3)
setImages(18-20)clearImages(39-42)setCurrentViewIndex(22-34)frontend/src/components/Media/ImageCard.tsx (1)
ImageCard(19-109)frontend/src/components/Media/MediaView.tsx (1)
MediaView(23-214)frontend/src/components/Albums/AddImagesToAlbumDialog.tsx (1)
AddImagesToAlbumDialog(23-210)
frontend/src/pages/Album/Album.tsx (7)
frontend/src/features/albumSelectors.ts (1)
selectAlbums(5-5)frontend/src/features/albumsSlice.ts (2)
removeAlbum(38-46)setAlbums(21-23)frontend/src/types/Album.ts (1)
Album(1-10)frontend/src/hooks/useQueryExtension.ts (2)
usePictoQuery(80-108)usePictoMutation(26-78)frontend/src/api/api-functions/albums.ts (2)
getAllAlbums(15-20)deleteAlbum(67-72)frontend/src/features/loaderSlice.ts (2)
hideLoader(21-24)showLoader(17-20)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)
🪛 GitHub Actions: PR Check
backend/app/routes/albums.py
[error] 1-1: API response for /albums endpoints is missing the 'is_hidden' field in album objects. Tests expect this field in the JSON response but it is not present.
[error] 1-1: db_get_all_albums() is being called without a boolean argument, while tests expect it to be called with a 'show_hidden' flag (True/False). This mismatch causes test failures.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tauri Build Check (windows-latest)
🔇 Additional comments (11)
frontend/src/components/EmptyStates/EmptyAlbumsState.tsx (1)
1-2: LGTM!The icon imports are appropriate for the empty state component.
frontend/src/api/apiEndpoints.ts (1)
33-46: LGTM! Clean endpoint definitions.The album endpoints follow the established pattern and RESTful conventions. The implementation is consistent with existing endpoint groups in the file.
frontend/src/app/store.ts (1)
9-20: LGTM! Proper Redux integration.The albums reducer is correctly imported and integrated into the store configuration following the established pattern.
frontend/src/components/Albums/EditAlbumDialog.tsx (2)
92-112: Validation logic handles lock state transitions correctly.The validation properly distinguishes between:
- Locking an unlocked album (requires new password)
- Modifying a locked album (requires current password)
- Changing password on locked album (current password required, new password optional)
22-301: Well-implemented edit dialog with proper password state management.The component correctly handles the complex logic for editing locked/unlocked albums, including:
- Appropriate validation for different lock states
- Clear UI feedback with contextual labels
- Proper cleanup on close
frontend/src/components/Albums/AlbumCard.tsx (2)
26-55: Robust image handling with proper error fallback.The component correctly:
- Selects theme-appropriate placeholders
- Uses Tauri's
convertFileSrcfor local file paths- Prevents infinite error loops by setting
img.onerror = null
92-103: Clean presentation with proper text handling.The info section correctly:
- Truncates long album names
- Clamps description to 2 lines
- Pluralizes "photo/photos" appropriately
frontend/src/pages/Album/AlbumDetail.tsx (2)
215-221: Proper cleanup prevents state leaks.The cleanup effect correctly clears all album-related state on unmount, preventing stale data from persisting.
43-437: Comprehensive album detail page with good UX patterns.The component effectively handles:
- Password-protected album access
- Multiple viewing modes (normal, selection, media viewer)
- Bulk operations with clear visual feedback
- Context menus for per-image actions
- Proper state cleanup on unmount
frontend/src/api/api-functions/albums.ts (1)
1-151: LGTM! Well-structured API layer.The API functions are well-documented, properly typed, and use appropriate HTTP methods. The consistent error handling pattern and clear separation of concerns make this module easy to maintain.
backend/app/database/albums.py (1)
65-82: LGTM! Well-implemented migration.The migration function properly checks for column existence before altering the table, preventing errors on repeated runs. Good defensive programming.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/tests/test_albums.py (2)
467-488: Fix inconsistent mock return tuple shape.Line 477 uses
tuple(mock_db_album.values())which creates a 5-element tuple from the fixture dict. However, other tests throughout this file explicitly construct 6-element tuples that includeimage_countas the last element. This inconsistency could cause test failures ifdb_get_albumexpects to return 6 elements.Apply this diff to use a consistent 6-element tuple:
- with patch("app.routes.albums.db_get_album") as mock_get, patch( - "app.routes.albums.db_remove_images_from_album" - ) as mock_remove_bulk: - mock_get.return_value = tuple(mock_db_album.values()) + album_tuple = ( + mock_db_album["album_id"], + mock_db_album["album_name"], + mock_db_album["description"], + int(mock_db_album["is_locked"]), + mock_db_album["password_hash"], + 0, # image_count + ) + + with patch("app.routes.albums.db_get_album") as mock_get, patch( + "app.routes.albums.db_remove_images_from_album" + ) as mock_remove_bulk: + mock_get.return_value = album_tuple response = client.request( "DELETE", f"/albums/{album_id}/images", json=image_ids_to_remove )
99-105: Add missingpassword_hashfield to the 6-element tuple fordb_get_album_by_namemock.The
db_get_album_by_namefunction's SQL query selects 6 fields (album_id,album_name,description,is_locked,password_hash,cover_image_path), but this mock returns only 5 elements. Update the mock to include thepassword_hashfield as the 5th element:mock_get_by_name.return_value = ( "existing-id", "Existing Album", "desc", 0, None, # password_hash None, # cover_image_path )
🧹 Nitpick comments (1)
backend/tests/test_albums.py (1)
149-185: Consider renaming test function for consistency.The test function name still contains "hidden" (
test_get_all_albums_include_hidden), while the docstring and implementation have been updated to use "locked" terminology. For consistency, consider renaming this totest_get_all_albums_include_locked.- def test_get_all_albums_include_hidden(self, mock_db_album, mock_db_locked_album): + def test_get_all_albums_include_locked(self, mock_db_album, mock_db_locked_album):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/test_albums.py(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/test_albums.py (1)
backend/tests/test_folders.py (1)
client(76-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Backend Tests
🔇 Additional comments (3)
backend/tests/test_albums.py (3)
23-42: LGTM! Fixture updates are correct.The fixtures have been correctly updated to reflect the migration from
is_hiddentois_locked, and the naming change frommock_db_hidden_albumtomock_db_locked_albumis appropriate.
53-352: Test updates for CRUD operations look good.The test methods in
TestAlbumRouteshave been appropriately updated to reflect the migration fromis_hiddentois_locked. The parametrized tests provide good coverage of different scenarios (public/locked albums, password validation), and most mock return tuples consistently use the 6-element format withcover_image_pathandimage_countfields.
355-466: Test updates for image management operations look good.The test methods in
TestAlbumImageManagementcorrectly use the updated 6-element tuple format for mock album data, ensuring consistency with the new album schema that includesis_locked,cover_image_path, andimage_countfields.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend/app/database/albums.py (1)
107-118: LGTM! Past issue with unused parameter resolved.The removal of the
show_hidden/show_lockedparameter addresses the previous review comment about an ignored parameter. The function now has clear, unambiguous behavior—it always returns all albums.backend/app/routes/albums.py (2)
128-182: LGTM! Update endpoint correctly refactored.The endpoint correctly uses
is_lockedthroughout, and there's no duplicate route decorator issue (which was mentioned in a previous review). The password validation logic for locked albums is properly maintained.
376-430: LGTM! Cover image endpoint is well-implemented.The new endpoint has comprehensive validation:
- Album existence check
- Image membership verification (image must be in the album)
- Image existence validation
The use of
db_get_image_pathmaintains proper architectural separation (addressing a previous review comment about inline database queries).
🧹 Nitpick comments (4)
frontend/src/components/Albums/AddImagesToAlbumDialog.tsx (2)
34-44: Remove redundant state duplication.The
allImagesstate (line 32) unnecessarily duplicatesimagesData.data. This adds complexity, extra re-renders, and a type assertion that could be unsafe.Apply this diff to use
imagesData.datadirectly:- const [allImages, setAllImages] = useState<Image[]>([]); const { data: imagesData, isLoading } = usePictoQuery({ queryKey: ['images'], queryFn: () => fetchAllImages(), enabled: isOpen, }); - useEffect(() => { - if (imagesData?.data) { - setAllImages(imagesData.data as Image[]); - } - }, [imagesData]);Then update line 104 to use
imagesData?.datadirectly:- const filteredImages = allImages.filter((image) => { + const filteredImages = (imagesData?.data as Image[] || []).filter((image) => {
46-96: Simplify by removing manual loader dispatches.The component uses both manual loader dispatches (
showLoader/hideLoaderat lines 50, 61, 94) and theisPendingflag from the mutation. This is redundant—isPendingalready provides the loading state needed for UI feedback.Remove the manual loader dispatches and rely solely on
isPending:const handleSubmit = () => { if (selectedImages.size === 0) { dispatch( showInfoDialog({ title: 'No Images Selected', message: 'Please select at least one image to add to the album.', variant: 'info', }), ); return; } - dispatch(showLoader('Adding images to album...')); addImagesMutate({ image_ids: Array.from(selectedImages) }); };And remove
hideLoadercalls from the mutation callbacks (lines 50, 61):const { mutate: addImagesMutate, isPending } = usePictoMutation({ mutationFn: (data: { image_ids: string[] }) => addImagesToAlbum(albumId, data), onSuccess: () => { - dispatch(hideLoader()); dispatch( showInfoDialog({ title: 'Success', message: `Added ${selectedImages.size} image${selectedImages.size > 1 ? 's' : ''} to album!`, variant: 'info', }), ); handleClose(); }, onError: (error: any) => { - dispatch(hideLoader()); dispatch( showInfoDialog({ title: 'Error', message: error?.message || 'Failed to add images. Please try again.', variant: 'error', }), ); }, });The button states at lines 200 and 206 already use
isPendingcorrectly.backend/app/database/albums.py (1)
220-322: Consider using the context manager consistently.While not critical, there's an inconsistency in connection handling:
db_update_album_cover_imageanddb_get_image_pathuse manual connection management, while some other functions (e.g.,db_delete_album,db_add_images_to_album) use theget_db_connection()context manager defined at the top of the file. The context manager provides retry logic and better error handling.Consider refactoring these functions to use the context manager:
def db_update_album_cover_image(album_id: str, cover_image_path: str): """Update the cover image path for an album""" - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() - try: + with get_db_connection() as conn: + cursor = conn.cursor() cursor.execute( "UPDATE albums SET cover_image_path = ? WHERE album_id = ?", (cover_image_path, album_id), ) - conn.commit() - finally: - conn.close()Apply similar changes to
db_get_image_pathand other functions using manual connection management.backend/app/routes/albums.py (1)
43-45: Consider optimizing image count queries.The current implementation performs N+1 queries: one to fetch all albums, then one per album to count images. For large album collections, this could be slow.
Consider adding a database function that returns albums with image counts in a single query:
def db_get_all_albums_with_counts(): """Get all albums with their image counts in a single query""" conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() try: cursor.execute( """ SELECT a.album_id, a.album_name, a.description, a.is_locked, a.password_hash, a.cover_image_path, COUNT(ai.image_id) as image_count FROM albums a LEFT JOIN album_images ai ON a.album_id = ai.album_id GROUP BY a.album_id """ ) return cursor.fetchall() finally: conn.close()This reduces the operation from O(n) queries to O(1).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/app/database/albums.py(11 hunks)backend/app/routes/albums.py(9 hunks)backend/app/schemas/album.py(3 hunks)backend/tests/test_albums.py(14 hunks)frontend/src/components/Albums/AddImagesToAlbumDialog.tsx(1 hunks)frontend/src/components/Albums/AlbumPasswordDialog.tsx(1 hunks)frontend/src/features/albumSelectors.ts(1 hunks)frontend/src/types/Album.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/features/albumSelectors.ts
- frontend/src/components/Albums/AlbumPasswordDialog.tsx
- backend/app/schemas/album.py
- frontend/src/types/Album.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/Albums/AddImagesToAlbumDialog.tsx (7)
frontend/src/types/Album.ts (1)
AddImagesToAlbumDialogProps(66-71)frontend/src/types/Media.ts (1)
Image(13-22)frontend/src/hooks/useQueryExtension.ts (2)
usePictoQuery(80-108)usePictoMutation(26-78)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(5-14)frontend/src/api/api-functions/albums.ts (1)
addImagesToAlbum(79-88)frontend/src/features/loaderSlice.ts (2)
hideLoader(21-24)showLoader(17-20)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-30)
backend/app/routes/albums.py (2)
backend/app/schemas/album.py (5)
SetCoverImageRequest(60-61)GetAlbumsResponse(69-71)Album(6-12)ErrorResponse(94-97)SuccessResponse(89-91)backend/app/database/albums.py (5)
db_update_album_cover_image(220-231)db_get_image_path(313-322)db_get_all_albums(107-118)db_get_album_images(234-244)db_get_album(135-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tauri Build Check (windows-latest)
🔇 Additional comments (18)
frontend/src/components/Albums/AddImagesToAlbumDialog.tsx (6)
1-21: LGTM!Imports are well-organized and all dependencies are properly declared.
23-32: LGTM!Component props and state initialization are correct. Using a
Setfor selectedImages is an efficient choice for fast lookups and toggles.
72-102: LGTM!Handler functions are correctly implemented:
handleImageToggleproperly creates a new Set for immutabilityhandleSubmitvalidates selection before proceedinghandleCloseproperly resets all local state before closing
104-108: LGTM!Filtering logic is correct with proper handling of edge cases (undefined from
.pop()via|| ''). Case-insensitive search provides good UX.
161-170: Past review feedback addressed—theme-aware placeholders implemented correctly.The previous review comment about using non-existent
/placeholder.svghas been resolved. The current implementation correctly uses theme-aware placeholders (/placeholder-album.svgfor dark mode,/placeholder-album-light.svgfor light mode) viawindow.matchMedia, aligning with the project's existing assets.
110-215: LGTM!UI structure and rendering logic are well-implemented:
- Proper loading, empty, and error states
- Accessible search with icon
- Responsive grid layout with visual selection feedback
- Clear user feedback with selected count and dynamic button text
- Buttons correctly disabled based on pending state and selection
backend/tests/test_albums.py (4)
23-42: LGTM! Fixture renaming aligns with schema changes.The fixture renaming from
mock_db_hidden_albumtomock_db_locked_albumand the field update fromis_hiddentois_lockedcorrectly reflect the backend schema changes.
53-68: LGTM! Test parameterization covers locked and unlocked scenarios.The test cases correctly exercise both
is_locked=False(public) andis_locked=True(password-protected) album creation flows.
119-171: LGTM! Mock return data correctly reflects database schema changes.The addition of
cover_image_pathandimage_countplaceholders to mock return tuples aligns with the updated database layer, and allis_hiddenreferences have been correctly replaced withis_locked.
329-353: LGTM! Test expectations correctly updated for schema changes.The test correctly constructs album tuples with
int(mock_db_album["is_locked"])for SQLite compatibility and includes theimage_countplaceholder, consistent with the database layer changes.backend/app/database/albums.py (4)
42-62: LGTM! Album table schema correctly updated.The schema now includes
is_locked,password_hash, andcover_image_pathfields, aligning with the feature requirements for locked albums and cover image management.
149-211: LGTM! Parameter renaming and SQL updates are consistent.The renaming of
is_hiddentois_lockedis correctly applied in bothdb_insert_albumanddb_update_album, with proper boolean-to-integer conversion for SQLite compatibility.
220-231: LGTM! Cover image update function is straightforward.The
db_update_album_cover_imagefunction correctly updates the cover image path for an album.
313-322: LGTM! Image path retrieval function is correct.The
db_get_image_pathfunction appropriately queries theimagestable to retrieve the file path for a given image ID.backend/app/routes/albums.py (4)
1-31: LGTM! Imports correctly updated for new functionality.The addition of
SetCoverImageRequest,db_update_album_cover_image, anddb_get_image_pathsupports the new cover image endpoint and addresses the previous inline database query issue.
36-57: LGTM! Function correctly updated for schema changes.The removal of the parameter and the addition of image count and cover image path support align with the backend changes. The safe indexing
album[5] if len(album) > 5 else Nonehandles potential schema migration cases gracefully.
91-125: LGTM! Album detail endpoint correctly updated.The endpoint properly incorporates image count calculation and cover image path, with consistent error handling and safe field access.
211-267: LGTM! Image retrieval endpoint correctly updated.The endpoint properly checks
is_lockedstatus and requires password authentication for locked albums, with clear error messages.
| def db_migrate_add_cover_image_column() -> None: | ||
| """Add cover_image_path column to existing albums table if it doesn't exist""" | ||
| conn = None | ||
| try: | ||
| conn = sqlite3.connect(DATABASE_PATH) | ||
| cursor = conn.cursor() | ||
|
|
||
| # Check if column exists | ||
| cursor.execute("PRAGMA table_info(albums)") | ||
| columns = [column[1] for column in cursor.fetchall()] | ||
|
|
||
| if "cover_image_path" not in columns: | ||
| cursor.execute("ALTER TABLE albums ADD COLUMN cover_image_path TEXT") | ||
| conn.commit() | ||
| print("Added cover_image_path column to albums table") | ||
| finally: | ||
| if conn is not None: | ||
| conn.close() |
There was a problem hiding this comment.
🧩 Analysis chain
Verify migration function is invoked.
The migration function logic is correct, but ensure it's being called during application initialization or in a migration script to actually apply the schema change to existing databases.
Run this script to check if the migration function is invoked:
🏁 Script executed:
#!/bin/bash
# Check if db_migrate_add_cover_image_column is called anywhere
rg -n "db_migrate_add_cover_image_column" --type=py -C 3Length of output: 421
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for migration-related patterns in initialization and main files
rg -n "(migrate|initialization|startup)" --type=py -C 2 backend/app/__init__.py backend/app/database/__init__.py backend/app/main.py 2>/dev/null || echo "No direct matches"
# Also check if there's any file that imports this migration function
rg -n "from.*albums.*import|import.*db_migrate" --type=py
Length of output: 423
---
🏁 Script executed:
```shell
#!/bin/bash
# Check where db_create functions are called in main.py to understand the initialization flow
sed -n '15,50p' backend/main.py
Length of output: 1412
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full lifespan function to see complete initialization
sed -n '40,70p' backend/main.py
Length of output: 921
Add migration function to application startup sequence.
The db_migrate_add_cover_image_column function is defined but never called, so the cover_image_path column won't be added to existing databases. Import and invoke it during application initialization in backend/main.py within the lifespan context manager, after db_create_albums_table() is called (around line 54).
🤖 Prompt for AI Agents
In backend/main.py around line 54 inside the lifespan context manager, the
migration function db_migrate_add_cover_image_column from
backend.app.database.albums is not invoked; import
db_migrate_add_cover_image_column at the top of the file and call it immediately
after the existing db_create_albums_table() invocation so the migration runs on
startup for existing databases (keep it synchronous, handle/let exceptions
propagate as with db_create_albums_table()).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/Albums/EditAlbumDialog.tsx (1)
38-40: Prefer explicit typing overanyfor type safety.The mutation function parameter
datais typed asany, which bypasses TypeScript's type checking. Consider using theUpdateAlbumRequesttype instead.Apply this diff to improve type safety:
- const { mutate: updateAlbumMutate, isPending } = usePictoMutation({ - mutationFn: ({ albumId, data }: { albumId: string; data: any }) => - updateAlbum(albumId, data), + const { mutate: updateAlbumMutate, isPending } = usePictoMutation({ + mutationFn: ({ albumId, data }: { albumId: string; data: UpdateAlbumRequest }) => + updateAlbum(albumId, data),You'll also need to import the type:
import { UpdateAlbumRequest } from '@/types/Album';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/Albums/CreateAlbumDialog.tsx(1 hunks)frontend/src/components/Albums/EditAlbumDialog.tsx(1 hunks)frontend/src/features/albumSelectors.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/Albums/CreateAlbumDialog.tsx
- frontend/src/features/albumSelectors.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/Albums/EditAlbumDialog.tsx (5)
frontend/src/types/Album.ts (1)
EditAlbumDialogProps(46-51)frontend/src/hooks/useQueryExtension.ts (1)
usePictoMutation(26-78)frontend/src/api/api-functions/albums.ts (1)
updateAlbum(52-61)frontend/src/features/loaderSlice.ts (2)
hideLoader(21-24)showLoader(17-20)frontend/src/features/infoDialogSlice.ts (1)
showInfoDialog(16-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: 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 (3)
frontend/src/components/Albums/EditAlbumDialog.tsx (3)
80-90: LGTM!The form population logic correctly pre-fills the form fields from the album prop while resetting password fields to empty strings for security. The effect dependency on
[album]is appropriate.
92-112: LGTM!The validation logic correctly handles all scenarios:
- Requires name field
- Requires current password when modifying a locked album
- Requires new password when locking a previously unlocked album
The use of optional chaining (
album?.is_locked) safely handles null cases.
205-281: LGTM!The password fields logic is well-implemented:
- Current password is shown and required only for albums that were originally locked
- New password field appears when the lock toggle is enabled
- Labels and placeholders are contextual and user-friendly
- Required indicators correctly match the validation rules
The UX clearly communicates when passwords are optional vs. required.
| const requestData: any = { | ||
| name: formData.name.trim(), | ||
| ...(formData.description.trim() && { | ||
| description: formData.description.trim(), | ||
| }), | ||
| is_locked: formData.is_locked, | ||
| }; |
There was a problem hiding this comment.
Fix the description clearing logic.
The current implementation only includes the description field in the request when it has content (line 125-127). This prevents users from clearing/emptying the description field. If a user deletes all text from the description textarea, the field won't be sent to the backend, causing the old description to persist.
Apply this diff to allow clearing the description:
const requestData: any = {
name: formData.name.trim(),
- ...(formData.description.trim() && {
- description: formData.description.trim(),
- }),
+ description: formData.description.trim(),
is_locked: formData.is_locked,
};This ensures the description field is always sent, allowing users to clear it by submitting an empty string.
📝 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 requestData: any = { | |
| name: formData.name.trim(), | |
| ...(formData.description.trim() && { | |
| description: formData.description.trim(), | |
| }), | |
| is_locked: formData.is_locked, | |
| }; | |
| const requestData: any = { | |
| name: formData.name.trim(), | |
| description: formData.description.trim(), | |
| is_locked: formData.is_locked, | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/components/Albums/EditAlbumDialog.tsx around lines 123 to 129,
the requestData object currently only includes description when non-empty which
prevents clearing it; change the object so description is always present (use
the trimmed description string, allowing it to be an empty string) instead of
conditionally spreading it, so the backend receives an explicit empty value when
the user clears the textarea.
1 participant
Description
This PR implements a complete Albums feature , including both backend API endpoints and a fully functional frontend UI.
Features Implemented
Backend
Frontend
Technical Details
Architecture
albumsSlice.tsfor persistent stateusePictoQueryandusePictoMutationhooksinfoDialogSlice.tsfor messages andloaderSlice.tsfor loading statesKey Changes
albumsSlice.ts) with 11 actions for album state managementalbumSelectors.ts) usingcreateSelectoralbums.ts)AlbumCard.tsx- Album display with lock iconCreateAlbumDialog.tsx- New album creationEditAlbumDialog.tsx- Album editing with lock managementDeleteConfirmDialog.tsx- Delete confirmationAlbumPasswordDialog.tsx- Password entry for locked albumsAddImagesToAlbumDialog.tsx- Image managementAlbum.tsx- Albums list viewAlbumDetail.tsx- Album detail view with imagesTesting Checklist
Related Issues
Tasks
usePictoQueryandusePictoMutationto handle API calling.infoDialogSlice.tsandloaderSlice.tsto display dialog messages and loaders, respectively.pictopy-album-issue_.2.mp4
Summary by CodeRabbit
New Features
UI
Behavior Changes