[Feat]: Image level deletion , Soft Delete Images with Undo, Restore & Permanent Delete#801
Conversation
|
|
WalkthroughThis PR introduces comprehensive soft-delete functionality across the application. Backend adds deletion tracking fields ( Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Gallery as GalleryToolbar
participant Redux as Redux State
participant API as API Layer
participant Backend as Backend API
participant DB as Database
User->>Gallery: Selects images
Gallery->>Redux: dispatch toggleImageSelection
Redux->>Redux: Add/remove from selectedImages
User->>Gallery: Click Delete
Gallery->>API: softDeleteImages(imageIds)
API->>Backend: POST /soft-delete
Backend->>DB: db_soft_delete_images()
DB->>DB: UPDATE images SET is_deleted=1, deleted_at=NOW()
DB->>DB: INSERT INTO action_history (delete, album_remove)
DB-->>Backend: ✓
Backend-->>API: SoftDeleteResponse
API-->>Gallery: response.data
Gallery->>Redux: dispatch markImagesAsDeleted(ids)
Redux->>Redux: Update images, clear selectedImages
Gallery->>Redux: dispatch setUndoState(lastDeletedIds)
Redux->>Redux: Enable showUndo notification
User->>Gallery: Click Undo (in notification)
Gallery->>API: restoreImages(lastDeletedImages)
API->>Backend: POST /restore
Backend->>DB: db_restore_images()
DB->>DB: UPDATE images SET is_deleted=0, deleted_at=NULL
DB->>DB: INSERT INTO action_history (restore)
DB-->>Backend: ✓
Backend-->>API: RestoreResponse
API-->>Gallery: response.data
Gallery->>Redux: dispatch markImagesAsRestored(ids)
Gallery->>Redux: dispatch clearUndoState()
Redux->>Redux: Disable showUndo, update images
sequenceDiagram
actor User
participant History as History Page
participant Redux as Redux State
participant API as API Layer
participant Backend as Backend API
participant DB as Database
User->>History: Navigate to History
History->>API: fetchDeletedImages()
API->>Backend: GET /deleted
Backend->>DB: db_get_deleted_images()
DB->>DB: SELECT * FROM images WHERE is_deleted=1
DB-->>Backend: [deleted image records]
Backend-->>API: GetDeletedImagesResponse
API-->>History: response.data (deleted images)
History->>Redux: Set deletedImages in state
User->>History: Selects deleted images
History->>Redux: dispatch selectImage / toggleImageSelection
Redux->>Redux: Add to selectedImages
alt Restore Selected
User->>History: Click Restore
History->>API: restoreImages(selectedIds)
API->>Backend: POST /restore
Backend->>DB: db_restore_images()
DB-->>Backend: ✓
Backend-->>API: RestoreResponse
API-->>History: restored_count
History->>API: Refetch fetchDeletedImages()
else Permanently Delete
User->>History: Click Delete Permanently
History->>API: permanentDeleteImages(selectedIds)
API->>Backend: POST /permanent-delete
Backend->>DB: db_permanently_delete_images()
DB->>DB: DELETE FROM images WHERE id IN (...)
DB->>DB: INSERT INTO action_history (permanent_delete)
DB-->>Backend: ✓
Backend-->>API: PermanentDeleteResponse
API-->>History: deleted_count
History->>API: Refetch fetchDeletedImages()
end
History->>Redux: Refetch clears selectedImages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
|
|
1 similar 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 (10)
backend/migrate_soft_delete.py (1)
54-60: Consider a composite index for soft-delete queries.The cleanup query in
db_permanently_delete_old_imagesfilters by bothis_deleted = 1 AND deleted_at < ?. A composite index would serve this query better than a single-column index onis_deleted.🔎 Suggested improvement
# Create index on images for soft delete queries cursor.execute( """ - CREATE INDEX IF NOT EXISTS idx_images_is_deleted - ON images(is_deleted) + CREATE INDEX IF NOT EXISTS idx_images_soft_delete + ON images(is_deleted, deleted_at) """ )frontend/src/components/Gallery/GalleryToolbar.tsx (1)
34-44: Use typed selectors orRootStateinstead ofany.These inline selectors bypass TypeScript's type checking. Consider using the existing typed selectors pattern or importing
RootStatefor proper type safety.🔎 Suggested improvement
+import { RootState } from '@/store'; // or wherever RootState is exported + - const selectedImages = useSelector( - (state: any) => state.images.selectedImages - ); + const selectedImages = useSelector( + (state: RootState) => state.images.selectedImages + ); - const showUndo = useSelector( - (state: any) => state.images.showUndo - ); + const showUndo = useSelector( + (state: RootState) => state.images.showUndo + ); - const lastDeletedImages = useSelector( - (state: any) => state.images.lastDeletedImages - ); + const lastDeletedImages = useSelector( + (state: RootState) => state.images.lastDeletedImages + );Alternatively, add these selectors to
imageSelectors.tsfor consistency with the existing pattern.frontend/src/api/api-functions/images.ts (1)
9-11: Consider using a typed params object instead ofany.Using
anybypasses TypeScript's type checking. A simple inline type or interface would improve type safety.🔎 Proposed fix
- const params: any = {}; + const params: { tagged?: boolean; include_deleted?: boolean } = {};frontend/src/pages/History/History.tsx (2)
62-62: Type assertion on potentially empty/undefined data.If the API returns an unexpected shape (e.g.,
dataisundefinedor not an array), theas Image[]assertion will silently pass invalid data. Consider adding a runtime check or using a more defensive approach.🔎 Proposed fix
- const deletedImages = (deletedImagesData?.data ?? []) as Image[]; + const deletedImages: Image[] = Array.isArray(deletedImagesData?.data) + ? deletedImagesData.data + : [];
15-16: Local selection state differs from Redux pattern used in ImageCard.The History page manages selection with local
useState, whileImageCardderives selection from Redux state (selectSelectedImages). This inconsistency could cause confusion and potential bugs if components are reused across contexts. Consider aligning on one approach.Also applies to: 66-80
frontend/src/components/Media/ImageCard.tsx (1)
111-121: Inconsistent indentation in delete button.The delete button uses different indentation compared to the favourite button above it. Consider aligning the formatting for consistency.
🔎 Proposed fix
<Button - variant="ghost" - size="icon" - className="rounded-full bg-red-500/80 p-2.5 text-white hover:bg-red-600" - onClick={(e) => { - e.stopPropagation(); - deleteSingleImage(image.id); - }} - > - <Trash2 className="h-5 w-5" /> - </Button> + variant="ghost" + size="icon" + className="rounded-full bg-red-500/80 p-2.5 text-white hover:bg-red-600" + onClick={(e) => { + e.stopPropagation(); + deleteSingleImage(image.id); + }} + > + <Trash2 className="h-5 w-5" /> + </Button>frontend/src/features/imageSlice.ts (1)
114-122: Inconsistent indentation in undo reducers.These reducers have no leading indentation, breaking the consistent style used elsewhere in the file.
🔎 Proposed fix
- setUndoState(state, action: PayloadAction<string[]>) { - state.lastDeletedImages = action.payload; - state.showUndo = true; -}, - -clearUndoState(state) { - state.lastDeletedImages = []; - state.showUndo = false; -}, + setUndoState(state, action: PayloadAction<string[]>) { + state.lastDeletedImages = action.payload; + state.showUndo = true; + }, + + clearUndoState(state) { + state.lastDeletedImages = []; + state.showUndo = false; + },backend/app/routes/images.py (3)
186-201:deleted_countmay not reflect actual deletions.The response uses
len(req.image_ids)asdeleted_countregardless of how many images were actually soft-deleted. If some IDs don't exist or are already deleted, the count will be misleading.Consider returning the actual count of affected rows from the database layer if accurate reporting is needed.
280-297: Consider restricting permanent deletion to soft-deleted images only.The endpoint allows permanent deletion of any image, including those not in the soft-deleted state. Per the PR description, "Permanent deletion is restricted to the Recently Deleted view." Consider adding a check in
db_permanently_delete_imagesto only delete images whereis_deleted = 1to enforce this constraint at the data layer.🔎 Proposed database layer fix in backend/app/database/images.py
try: - # Permanently delete the images (this will cascade to image_classes) + # Permanently delete only soft-deleted images (this will cascade to image_classes) placeholders = ",".join("?" for _ in image_ids) cursor.execute( - f"DELETE FROM images WHERE id IN ({placeholders})", + f"DELETE FROM images WHERE id IN ({placeholders}) AND is_deleted = 1", image_ids, )
306-321: Consider validatingdaysparameter.The
daysparameter accepts any integer, including negative values which could unintentionally delete all soft-deleted images (since all would be "older" than a future date). Consider adding validation withQuery(30, ge=1, description=...).🔎 Proposed fix
@router.post("/cleanup", response_model=CleanupOldImagesResponse) def cleanup_old_images( - days: int = Query(30, description="Days after which to permanently delete") + days: int = Query(30, ge=1, description="Days after which to permanently delete") ):
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
backend/app/database/images.py(11 hunks)backend/app/routes/images.py(6 hunks)backend/cleanup_old_images.py(1 hunks)backend/migrate_soft_delete.py(1 hunks)docs/backend/backend_python/openapi.json(9 hunks)frontend/src/api/api-functions/images.ts(1 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/components/Gallery/GalleryToolbar.tsx(1 hunks)frontend/src/components/Gallery/HistoryToolbar.tsx(1 hunks)frontend/src/components/Gallery/UndoNotification.tsx(1 hunks)frontend/src/components/Media/ImageCard.tsx(3 hunks)frontend/src/components/Navigation/Sidebar/AppSidebar.tsx(2 hunks)frontend/src/constants/routes.ts(1 hunks)frontend/src/features/imageSelectors.ts(1 hunks)frontend/src/features/imageSlice.ts(2 hunks)frontend/src/hooks/useDeleteImage.ts(1 hunks)frontend/src/pages/History/History.tsx(1 hunks)frontend/src/pages/Home/Home.tsx(2 hunks)frontend/src/routes/AppRoutes.tsx(2 hunks)frontend/src/types/Media.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
frontend/src/hooks/useDeleteImage.ts (3)
frontend/src/api/api-functions/images.ts (1)
softDeleteImages(19-25)frontend/src/features/imageSlice.ts (2)
markImagesAsDeleted(94-102)setUndoState(114-117)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)
frontend/src/components/Gallery/GalleryToolbar.tsx (5)
frontend/src/features/imageSelectors.ts (5)
selectIsSelectionMode(15-16)selectSelectedImagesCount(18-21)selectAreAllImagesSelected(29-33)selectIsAnyImageSelected(35-38)selectImages(5-7)frontend/src/api/api-functions/images.ts (2)
softDeleteImages(19-25)restoreImages(27-33)frontend/src/features/imageSlice.ts (7)
markImagesAsDeleted(94-102)setUndoState(114-117)toggleSelectionMode(57-62)markImagesAsRestored(104-111)clearUndoState(119-122)deselectAllImages(89-91)selectAllImages(85-87)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)frontend/src/components/Gallery/UndoNotification.tsx (1)
UndoNotification(10-38)
frontend/src/routes/AppRoutes.tsx (2)
frontend/src/constants/routes.ts (1)
ROUTES(1-13)frontend/src/pages/History/History.tsx (1)
History(15-174)
frontend/src/api/api-functions/images.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-9)
backend/cleanup_old_images.py (1)
backend/app/database/images.py (1)
db_permanently_delete_old_images(651-707)
frontend/src/pages/Home/Home.tsx (1)
frontend/src/components/Gallery/GalleryToolbar.tsx (1)
GalleryToolbar(25-169)
frontend/src/components/Media/ImageCard.tsx (4)
frontend/src/features/imageSelectors.ts (2)
selectIsSelectionMode(15-16)selectSelectedImages(12-13)frontend/src/hooks/useDeleteImage.ts (1)
useDeleteImage(10-32)frontend/src/features/imageSlice.ts (1)
toggleImageSelection(75-83)frontend/src/lib/utils.ts (1)
cn(5-7)
frontend/src/features/imageSelectors.ts (1)
frontend/src/app/store.ts (1)
RootState(22-22)
backend/app/database/images.py (1)
backend/app/utils/images.py (1)
image_util_parse_metadata(496-513)
backend/app/routes/images.py (2)
backend/app/database/images.py (6)
db_get_all_images(167-274)db_soft_delete_images(484-544)db_restore_images(547-589)db_get_deleted_images(592-648)db_permanently_delete_images(710-752)db_permanently_delete_old_images(651-707)backend/app/utils/images.py (1)
image_util_parse_metadata(496-513)
🔇 Additional comments (27)
docs/backend/backend_python/openapi.json (1)
2454-2476: Ensure backend enforces consistency between is_deleted and deleted_at during soft-delete operations.While the schema correctly shows both
is_deletedanddeleted_atas optional/nullable fields (standard for soft-delete patterns), these fields should be logically coupled at the application level:deleted_atshould only contain a timestamp whenis_deletedis true. Verify the backend code that handles soft-delete operations enforces this constraint—for example, when marking an image as deleted, both fields should be updated atomically.backend/cleanup_old_images.py (1)
20-38: LGTM!The
main()function correctly invokesdb_permanently_delete_old_images, logs outcomes at appropriate levels, and exits with status 1 on failure. The logic is sound.frontend/src/api/apiEndpoints.ts (1)
4-8: LGTM!The new endpoints follow the existing naming conventions and are logically grouped under
imagesEndpoints.backend/migrate_soft_delete.py (1)
14-70: LGTM!The migration is idempotent, handles errors correctly with rollback, and properly closes the connection in the
finallyblock. Good use ofPRAGMA table_infoto check for existing columns.frontend/src/components/Gallery/GalleryToolbar.tsx (2)
46-56: LGTM on mutation logic.The mutations correctly dispatch Redux actions on success. The
markImagesAsDeletedandsetUndoStateare properly sequenced, and the restore flow clears undo state appropriately.Also applies to: 65-71
109-127: LGTM on conditional rendering.The three UI states (default, undo notification, selection mode) are cleanly separated and the logic flows correctly.
frontend/src/components/Gallery/HistoryToolbar.tsx (2)
4-10: LGTM!The props interface is well-defined with clear types, making the component's contract explicit.
12-63: LGTM!Clean implementation with proper disabled states and loading feedback. The conditional rendering for selection count and delete button is well-structured.
frontend/src/api/api-functions/images.ts (1)
19-59: LGTM!The new API functions for soft-delete, restore, fetch deleted, permanent delete, and cleanup are well-structured and consistent with the existing patterns. They correctly use POST for mutation operations and GET for fetching, and properly pass parameters to the API client.
frontend/src/features/imageSelectors.ts (1)
12-38: LGTM!The new selectors are well-implemented:
- Basic selectors for direct state access are appropriately simple
- Memoized selectors using
createSelectorprevent unnecessary re-rendersselectAreAllImagesSelectedcorrectly handles the edge case of empty images arrayfrontend/src/components/Media/ImageCard.tsx (1)
33-52: LGTM on Redux integration for selection.The component correctly reads selection state from Redux and dispatches
toggleImageSelectionwhen in selection mode, while preserving the originalonClickbehavior otherwise. ThehandleImageClickcallback has proper dependencies.frontend/src/features/imageSlice.ts (3)
94-102: Client-side timestamp may differ from server.
markImagesAsDeletedgeneratesdeleted_atusing the client's clock (new Date().toISOString()). If the server also sets this timestamp, there could be a mismatch. This is fine for optimistic UI updates, but ensure the data is refreshed from the server after the API call succeeds to sync the authoritative timestamp.
85-91:selectAllImagesonly selects from non-deleted images in state.This reducer selects IDs from
state.images, which typically contains non-deleted images. If the History page (Recently Deleted) tries to use this action, it won't select the deleted images displayed there. This is likely intentional, but verify this aligns with the expected behavior for the History page's "Select All" functionality (which currently uses local state anyway).
57-83: LGTM on selection mode reducers.The selection reducers are correctly implemented:
toggleSelectionModeproperly clears selection when exiting selection modeselectImage/deselectImagehandle add/remove correctly with duplicate checkstoggleImageSelectionefficiently usessplicefor removalbackend/app/database/images.py (8)
3-4: LGTM!The new imports for
Optional,datetime, andtimedeltaare appropriately added to support the soft-delete functionality with timestamps.
31-32: LGTM!The
ImageRecordTypedDict is correctly extended withis_deletedanddeleted_atfields to track soft-delete state.
48-57: LGTM!The
ActionHistoryRecordTypedDict is well-defined for audit logging purposes.
101-127: LGTM!The
action_historytable schema and indexes are well-designed. The descending index ontimestampoptimizes recent-first queries, and theis_deletedindex supports filtering operations.
144-145: LGTM!New image records are correctly initialized with
is_deleted = 0anddeleted_at = NULL.
167-214: LGTM!The
db_get_all_imagesfunction is correctly extended with theinclude_deletedparameter. The WHERE clause construction properly handles bothtaggedandinclude_deletedfilters with AND conditions.
592-648: LGTM!The
db_get_deleted_imagesfunction correctly retrieves soft-deleted images with proper metadata parsing and ordering by deletion timestamp.
651-707: LGTM!The cleanup function correctly identifies and permanently deletes old soft-deleted images. The ISO 8601 date format from Python's
isoformat()is compatible with SQLite's datetime comparison.backend/app/routes/images.py (5)
3-10: LGTM!The necessary database functions are properly imported for the new soft-delete functionality.
44-45: LGTM!The
ImageDatamodel is correctly extended withis_deletedanddeleted_atfields with appropriate defaults.
60-86: LGTM!The
get_all_imagesendpoint is correctly extended to support theinclude_deletedquery parameter with proper default behavior.
214-229: LGTM with the same caveat.The restore endpoint follows the same pattern as soft-delete. The
restored_countsimilarly reflects the request size rather than actual restorations.
238-267: LGTM!The
/deletedendpoint correctly retrieves soft-deleted images with appropriate field handling.
| conn.commit() | ||
| logger.info(f"Soft deleted {cursor.rowcount} image(s)") | ||
| return True |
There was a problem hiding this comment.
Logging statement uses incorrect rowcount.
At line 537, cursor.rowcount reflects the result of the last INSERT INTO action_history statement, not the number of soft-deleted images. Use len(image_ids) instead for accurate logging.
🔎 Proposed fix
conn.commit()
- logger.info(f"Soft deleted {cursor.rowcount} image(s)")
+ logger.info(f"Soft deleted {len(image_ids)} image(s)")
return True📝 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.
| conn.commit() | |
| logger.info(f"Soft deleted {cursor.rowcount} image(s)") | |
| return True | |
| conn.commit() | |
| logger.info(f"Soft deleted {len(image_ids)} image(s)") | |
| return True |
🤖 Prompt for AI Agents
In backend/app/database/images.py around lines 536 to 538, the logger.info call
uses cursor.rowcount which reflects the last INSERT INTO action_history, not the
number of images soft-deleted; replace that logging to use len(image_ids) (or an
explicit count variable) so the message accurately reports the number of
soft-deleted images, e.g. change the logger.info line to log len(image_ids) and
ensure image_ids is defined as the list of IDs being soft-deleted.
| conn.commit() | ||
| logger.info(f"Restored {cursor.rowcount} image(s)") | ||
| return True |
There was a problem hiding this comment.
Same rowcount logging issue as in db_soft_delete_images.
cursor.rowcount reflects the last INSERT, not the UPDATE. Use len(image_ids) for accurate logging.
🔎 Proposed fix
conn.commit()
- logger.info(f"Restored {cursor.rowcount} image(s)")
+ logger.info(f"Restored {len(image_ids)} image(s)")
return True📝 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.
| conn.commit() | |
| logger.info(f"Restored {cursor.rowcount} image(s)") | |
| return True | |
| conn.commit() | |
| logger.info(f"Restored {len(image_ids)} image(s)") | |
| return True |
🤖 Prompt for AI Agents
In backend/app/database/images.py around lines 581 to 583, the logger uses
cursor.rowcount which reflects the last INSERT rather than the number of images
restored; replace the rowcount usage with len(image_ids) (or the appropriate
list/collection variable holding the restored image IDs) when logging, ensuring
image_ids is in scope and represents the intended restored-set so the log
reports the correct count; keep the commit and return True as-is.
| conn.commit() | ||
| logger.info(f"Manually permanently deleted {cursor.rowcount} image(s)") | ||
| return True |
There was a problem hiding this comment.
Logging statement uses incorrect rowcount.
After the INSERT loop, cursor.rowcount is 1 (from the last INSERT), not the number of deleted images. Use len(image_ids) instead.
🔎 Proposed fix
conn.commit()
- logger.info(f"Manually permanently deleted {cursor.rowcount} image(s)")
+ logger.info(f"Manually permanently deleted {len(image_ids)} image(s)")
return True📝 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.
| conn.commit() | |
| logger.info(f"Manually permanently deleted {cursor.rowcount} image(s)") | |
| return True | |
| conn.commit() | |
| logger.info(f"Manually permanently deleted {len(image_ids)} image(s)") | |
| return True |
🤖 Prompt for AI Agents
In backend/app/database/images.py around lines 744 to 746, the logger uses
cursor.rowcount which reflects only the last INSERT rather than the total
deleted images; change the log to use len(image_ids) (or 0 if image_ids may be
None) so the message shows the actual number deleted, e.g.
logger.info(f"Manually permanently deleted {len(image_ids)} image(s)") after the
commit.
| @router.post("/delete") | ||
| def delete_image(req: DeleteRequest): | ||
| image_id = req.image_id | ||
| try: | ||
| success = db_soft_delete_images([image_id]) | ||
| if not success: | ||
| raise HTTPException( | ||
| status_code=404, detail="Image not found or failed to delete" | ||
| ) | ||
| return { | ||
| "success": True, | ||
| "image_id": image_id, | ||
| "is_deleted": True, | ||
| } | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"error in /delete route: {e}") | ||
| raise HTTPException(status_code=500, detail=f"Internal server error: {e}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the implementation of db_soft_delete_images
fd -t f -e py backend | xargs rg -l "def db_soft_delete_images"Repository: AOSSIE-Org/PictoPy
Length of output: 92
🏁 Script executed:
# Search for db_soft_delete_images definition and usage
rg -n "db_soft_delete_images" backend/Repository: AOSSIE-Org/PictoPy
Length of output: 388
🏁 Script executed:
# Find the DeleteRequest model definition
rg -n "class DeleteRequest" backend/Repository: AOSSIE-Org/PictoPy
Length of output: 126
🏁 Script executed:
# Read the db_soft_delete_images implementation
sed -n '484,520p' backend/app/database/images.pyRepository: AOSSIE-Org/PictoPy
Length of output: 1156
🏁 Script executed:
# Also check the DeleteRequest model definition
sed -n '136,145p' backend/app/routes/images.pyRepository: AOSSIE-Org/PictoPy
Length of output: 288
🏁 Script executed:
# Read more of the db_soft_delete_images function to see the complete implementation
sed -n '484,550p' backend/app/database/images.pyRepository: AOSSIE-Org/PictoPy
Length of output: 2002
🏁 Script executed:
# Check the bulk delete endpoint and how it handles the return value
sed -n '180,200p' backend/app/routes/images.pyRepository: AOSSIE-Org/PictoPy
Length of output: 766
🏁 Script executed:
# Look for any comments or TODOs about idempotency or deletion behavior
rg -n "idempotent|idempotency" backend/Repository: AOSSIE-Org/PictoPy
Length of output: 44
🏁 Script executed:
# Check if there are any related tests or specifications
fd -t f -e py backend | xargs rg -l "test.*delete.*image\|soft.*delete" | head -5Repository: AOSSIE-Org/PictoPy
Length of output: 44
Soft delete of non-existent image returns success.
db_soft_delete_images returns True even when no rows are updated (e.g., when the image ID doesn't exist). The endpoint will return success: True for non-existent images, which may confuse clients. The function logs the affected row count but does not validate it before returning success.
Consider verifying the image exists before deletion or checking the affected row count in the database function to return accurate success status.
🤖 Prompt for AI Agents
In backend/app/routes/images.py around lines 140 to 157, the route treats
db_soft_delete_images() returning True as proof the image was deleted even when
no rows were affected; change the logic to verify deletion actually occurred by
checking the affected row count (either change db_soft_delete_images to return
the number of affected rows or have it return False when zero rows were updated)
and then raise HTTPException(404) when no rows were deleted; alternatively first
verify the image exists (e.g., db_get_image_by_id) before attempting soft-delete
and return 404 if not found, and keep the 500 path for real errors.
| import os | ||
| import sys | ||
|
|
||
| from app.database.images import db_permanently_delete_old_images | ||
| from app.logging.setup_logging import get_logger | ||
|
|
||
| # Add project root to PYTHONPATH | ||
| sys.path.append(os.path.dirname(os.path.abspath(__file__))) | ||
|
|
||
| logger = get_logger(__name__) |
There was a problem hiding this comment.
Path manipulation must precede imports from local modules.
The sys.path.append on line 15 executes after the imports from app.database and app.logging on lines 11–12. If this script is run from outside the backend/ directory, the imports will fail with ModuleNotFoundError.
🔎 Proposed fix
#!/usr/bin/env python3
"""
Script to clean up old soft-deleted images.
Run this script periodically to permanently delete images that have been
soft deleted for more than 30 days.
"""
import os
import sys
+# Add project root to PYTHONPATH before local imports
+sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
+
from app.database.images import db_permanently_delete_old_images
from app.logging.setup_logging import get_logger
-# Add project root to PYTHONPATH
-sys.path.append(os.path.dirname(os.path.abspath(__file__)))
-
logger = get_logger(__name__)📝 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.
| import os | |
| import sys | |
| from app.database.images import db_permanently_delete_old_images | |
| from app.logging.setup_logging import get_logger | |
| # Add project root to PYTHONPATH | |
| sys.path.append(os.path.dirname(os.path.abspath(__file__))) | |
| logger = get_logger(__name__) | |
| import os | |
| import sys | |
| # Add project root to PYTHONPATH before local imports | |
| sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) | |
| from app.database.images import db_permanently_delete_old_images | |
| from app.logging.setup_logging import get_logger | |
| logger = get_logger(__name__) |
🤖 Prompt for AI Agents
In backend/cleanup_old_images.py around lines 8 to 17, the script appends the
project root to sys.path after importing local modules, which can cause
ModuleNotFoundError when run from outside backend; move the path manipulation to
the top of the file before any imports from app.*, compute the correct project
root (e.g., parent of this file) and insert it into sys.path (prefer
sys.path.insert(0, ...) to prioritize the project root), then keep the logger
and db imports as they are.
| useMutationFeedback(deleteMutation, { | ||
| loadingMessage: 'Deleting images...', | ||
| successMessage: `${selectedCount} images moved to recently deleted`, | ||
| showSuccess: true, | ||
| }); |
There was a problem hiding this comment.
Stale count in success message.
selectedCount is captured at render time. If the user deselects images before the mutation completes, the message may show an incorrect count. Consider using selectedImages.length directly or storing the count when initiating the mutation.
🔎 Suggested improvement
useMutationFeedback(deleteMutation, {
loadingMessage: 'Deleting images...',
- successMessage: `${selectedCount} images moved to recently deleted`,
+ successMessage: `Images moved to recently deleted`,
showSuccess: true,
});Or capture the count at mutation time and pass it through the mutation context.
🤖 Prompt for AI Agents
In frontend/src/components/Gallery/GalleryToolbar.tsx around lines 58 to 62, the
successMessage uses the render-time variable selectedCount which can be stale by
the time the mutation completes; change the implementation to capture the
selection count at mutation initiation and use that captured value for the
success message (or compute the count from selectedImages.length at the time you
call the mutation), e.g. create a local const countAtSubmit =
selectedImages.length when starting the delete mutation and pass that into
useMutationFeedback (or into the mutation context) so the success message uses
the captured count rather than the render-time selectedCount.
| onClick={(e) => { | ||
| console.log(image); | ||
| e.stopPropagation(); | ||
| handleToggleFavourite(); | ||
| }} |
There was a problem hiding this comment.
Remove debug console.log statement.
This appears to be a leftover debug statement that should be removed before merging.
🔎 Proposed fix
onClick={(e) => {
- console.log(image);
e.stopPropagation();
handleToggleFavourite();
}}📝 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.
| onClick={(e) => { | |
| console.log(image); | |
| e.stopPropagation(); | |
| handleToggleFavourite(); | |
| }} | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| handleToggleFavourite(); | |
| }} |
🤖 Prompt for AI Agents
In frontend/src/components/Media/ImageCard.tsx around lines 97 to 101, remove
the leftover debug console.log(image) from the onClick handler; keep
e.stopPropagation() and handleToggleFavourite() intact and ensure no other debug
console.log calls remain in this handler before committing.
| { name: 'Home', path: `/${ROUTES.HOME}`, icon: Home }, | ||
| { name: 'AI Tagging', path: `/${ROUTES.AI}`, icon: Sparkles }, | ||
| { name: 'Favourites', path: `/${ROUTES.FAVOURITES}`, icon: Heart }, | ||
| { name: 'Recently Deleted ', path: `/${ROUTES.HISTORY}`, icon: History }, |
There was a problem hiding this comment.
Remove trailing space in menu label.
The menu item name has a trailing space: "Recently Deleted ". This should be "Recently Deleted" for consistency.
🔎 Proposed fix
- { name: 'Recently Deleted ', path: `/${ROUTES.HISTORY}`, icon: History },
+ { name: 'Recently Deleted', path: `/${ROUTES.HISTORY}`, icon: History },📝 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.
| { name: 'Recently Deleted ', path: `/${ROUTES.HISTORY}`, icon: History }, | |
| { name: 'Recently Deleted', path: `/${ROUTES.HISTORY}`, icon: History }, |
🤖 Prompt for AI Agents
In frontend/src/components/Navigation/Sidebar/AppSidebar.tsx around line 53, the
menu item label has an unintended trailing space ("Recently Deleted "); remove
the trailing space so the name is exactly "Recently Deleted" to ensure
consistent labels and avoid rendering/spacing issues.
| useMutationFeedback(permanentDeleteMutation, { | ||
| loadingMessage: 'Permanently deleting images...', | ||
| successMessage: `${selectedImages.length} images permanently deleted`, | ||
| showSuccess: true, | ||
| }); |
There was a problem hiding this comment.
Success message captures stale selectedImages.length.
The selectedImages.length is captured when useMutationFeedback is called (on render), not when the mutation succeeds. If multiple images are deleted, the message may show an incorrect count.
🔎 Proposed fix - use dynamic message in onSuccess
Consider moving the success feedback into the mutation's onSuccess callback where you have access to the actual count of deleted items, or use a ref to track the count.
+ const deletedCountRef = useRef(0);
const permanentDeleteMutation = useMutation({
mutationFn: permanentDeleteImages,
- onSuccess: () => {
+ onSuccess: (_data, deletedIds) => {
+ deletedCountRef.current = deletedIds.length;
setSelectedImages([]);
refetchDeleted();
},
});
useMutationFeedback(permanentDeleteMutation, {
loadingMessage: 'Permanently deleting images...',
- successMessage: `${selectedImages.length} images permanently deleted`,
+ successMessage: `Images permanently deleted`,
showSuccess: true,
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/pages/History/History.tsx around lines 40-44, the successMessage
passed to useMutationFeedback captures selectedImages.length at render time and
can be stale; remove the static successMessage and instead produce the success
feedback inside the mutation's onSuccess handler (or capture the selected count
in a ref/variable immediately before calling the mutation and pass it through),
then call the feedback success function (or set showSuccess) with `${count}
images permanently deleted` using that actual count so the displayed number
reflects what was deleted.
| <Button | ||
| size="icon" | ||
| variant="destructive" | ||
| onClick={() => | ||
| permanentDeleteMutation.mutate([image.id]) | ||
| } | ||
| > | ||
| <Trash2 className="h-4 w-4" /> | ||
| </Button> |
There was a problem hiding this comment.
Per-image permanent delete uses same mutation as bulk delete.
When deleting a single image via the trash button, the success message from useMutationFeedback will still reference selectedImages.length, which may be 0 or unrelated to this single-item deletion. This creates a confusing UX.
Consider either creating a separate mutation for single-item deletion or adjusting the feedback mechanism to handle both cases.
🤖 Prompt for AI Agents
In frontend/src/pages/History/History.tsx around lines 151 to 159, the per-image
trash button calls the same permanentDeleteMutation used for bulk deletes which
causes the success feedback to reference selectedImages.length (possibly 0 or
unrelated); update the mutation handling so success feedback uses the actual
number of items deleted: either create a separate single-item delete mutation
for the trash button or change the existing mutation's onSuccess to derive the
deleted count from the mutation variables (e.g., the array passed to
mutate/mutateAsync) and use that count when calling useMutationFeedback so the
message accurately reflects single vs. bulk deletions.
1 participant
📌 Overview
Fixes #795
This PR introduces a safe and reversible image deletion workflow by implementing
soft delete instead of irreversible removal. Deleted images are moved to a
Recently Deleted section where users can undo, restore, or permanently delete
images.
The implementation follows modern gallery UX patterns and improves data safety,
recoverability, and user trust.
🎥 Demo Video
demo.mp4
Demonstrated in the video:
❗ Problem Statement
Previously:
This created a poor UX and high risk for users.
✅ Solution Implemented
🔹 1. Soft Delete (Non-destructive)
is_deleted = truedeleted_at = timestamp🔹 2. Recently Deleted Section
🔹 3. Undo Support (Immediate Recovery)
🔹 4. Permanent Delete (Explicit & Safe)
🔹 5. Backend Cleanup Support (Manual)
🏗️ Technical Details
Backend
is_deleted,deleted_atFrontend
🧠 Design Considerations
🧪 Testing & Validation
📈 Impact
📝 Notes for Reviewers
✅ Checklist