Right-Click Context Menu Actions for Images *(Favorite, Copy, View Details)*#900
Right-Click Context Menu Actions for Images *(Favorite, Copy, View Details)*#900JaYRaNa213 wants to merge 10 commits intoAOSSIE-Org:mainfrom
Conversation
|
|
📝 WalkthroughWalkthroughAdds a Radix-based context menu and UI plumbing for right-click image actions (copy-to-clipboard, view info), a Tauri clipboard command (arboard), backend image deletion with tombstone support and tests, plus related type/UI tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImageCard
participant Frontend as FrontendUI
participant Tauri as TauriBackend
participant Arboard
User->>ImageCard: Right-click → open context menu
User->>ImageCard: Select "Copy image"
ImageCard->>Frontend: invoke('copy_image_to_clipboard', path)
Frontend->>Tauri: Tauri invoke -> copy_image_to_clipboard(path)
Tauri->>Arboard: load file, convert to RGBA, write to clipboard
alt write success
Arboard-->>Tauri: Ok
Tauri-->>Frontend: Ok
Frontend->>User: show success InfoDialog
else write error
Arboard-->>Tauri: Err(msg)
Tauri-->>Frontend: Err(msg)
Frontend->>User: show error InfoDialog
end
sequenceDiagram
participant User
participant ImageCard
participant Gallery as ChronologicalGallery
participant Home
participant MediaInfoPanel
User->>ImageCard: Click "View Info" (or context action)
ImageCard->>Gallery: onViewInfo(image, index)
Gallery->>Home: forward onViewInfo(image, index)
Home->>Home: handleOpenInfo (set isInfoOpen, infoImage, infoIndex)
Home->>MediaInfoPanel: render modal with image/index
MediaInfoPanel->>User: display details
User->>MediaInfoPanel: Close
MediaInfoPanel->>Home: onClose → handleCloseInfo
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)frontend/src-tauri/src/main.rs (1)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
frontend/src/components/Media/ImageCard.tsx (2)
64-100: Consider wrapping handlers inuseCallbackfor consistency.
handleToggleFavouriteusesuseCallback, buthandleCopyandhandleViewInfodo not. While these handlers are used only in local event handlers (so performance impact is minimal), usinguseCallbackconsistently improves code predictability and would matter if these handlers were ever passed to memoized children.🔎 Suggested refactor
- const handleCopy = async () => { + const handleCopy = useCallback(async () => { try { await invoke('copy_image_to_clipboard', { path: image.path, }); dispatch( showInfoDialog({ title: 'Success', message: 'Image copied to clipboard', variant: 'success', }), ); } catch (err) { console.error(err); dispatch( showInfoDialog({ title: 'Error', message: 'Failed to copy image to clipboard', variant: 'error', }), ); } - }; + }, [dispatch, image.path]); - const handleViewInfo = () => { + const handleViewInfo = useCallback(() => { if (onViewInfo) { onViewInfo(image, imageIndex); } else if (onClick) { onClick(); } - }; + }, [onViewInfo, image, imageIndex, onClick]);
36-47: PropsonViewInfoandimageIndexare correctly paired in all current usages.Verification shows that every usage of
ImageCardwith theonViewInfoprop also providesimageIndexexplicitly. InChronologicalGallery, both props are consistently passed together with the correct index value. While the prop signature allows them to be used independently—which could theoretically lead toonViewInforeceiving a defaultimageIndexof0—this pattern does not occur in practice.The suggestions to enforce prop pairing (e.g., via TypeScript discriminated unions or runtime warnings) remain valid as preventative design measures, but are not currently necessary given how the component is used.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
frontend/package.jsonfrontend/src-tauri/Cargo.tomlfrontend/src-tauri/src/main.rsfrontend/src-tauri/tauri.conf.jsonfrontend/src/components/Dialog/InfoDialog.tsxfrontend/src/components/Media/ChronologicalGallery.tsxfrontend/src/components/Media/ImageCard.tsxfrontend/src/components/Media/MediaInfoPanel.tsxfrontend/src/components/ui/context-menu.tsxfrontend/src/pages/Home/Home.tsxfrontend/src/types/infoDialog.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src-tauri/src/main.rs (1)
frontend/src-tauri/src/services/mod.rs (1)
get_server_path(5-11)
frontend/src/components/Media/MediaInfoPanel.tsx (2)
frontend/src/types/Media.ts (1)
Image(13-23)frontend/src/lib/utils.ts (1)
cn(5-7)
frontend/src/components/Media/ChronologicalGallery.tsx (1)
frontend/src/types/Media.ts (1)
Image(13-23)
🔇 Additional comments (13)
frontend/src/components/Media/MediaInfoPanel.tsx (2)
13-13: LGTM! Good extensibility pattern.Adding the optional
classNameprop and applying it via thecnutility enables external styling while maintaining default classes. This is a clean implementation that follows React best practices.Also applies to: 21-21, 30-30, 67-67
111-112: AI summary inconsistency detected.The AI summary claims "Changes the Location button rendering to rely solely on the presence of longitude," but the code still checks both
latitudeandlongitudeon lines 111-112. The condition remainscurrentImage?.metadata?.latitude && currentImage?.metadata?.longitude, unchanged from the original logic.frontend/src/components/ui/context-menu.tsx (1)
1-199: LGTM! Well-structured context menu implementation.This is a clean, well-structured wrapper around Radix UI Context Menu primitives that follows best practices:
- Proper use of
forwardReffor all interactive components- Consistent
displayNameassignments for debugging- Type-safe prop spreading with TypeScript
- Appropriate use of the
cnutility for className composition- Support for advanced features (submenus, checkboxes, radio items, shortcuts)
- Accessible markup with proper focus states and data attributes
The implementation aligns with the shadcn/ui component pattern and integrates well with the Tailwind styling system.
frontend/package.json (1)
28-28: @radix-ui/react-context-menu@2.2.16 is compatible with React 19. The package was part of a Radix Primitives release (June 19, 2024) that added official React 19 support. Note: Some console warnings related to React 19 deprecation (element.ref issues) may appear in certain setups, but these are not blockers.frontend/src-tauri/Cargo.toml (1)
19-20: Dependencies are valid and safe.Verification confirms:
arboard3.4.0 and 3.4.1 are available on crates.ioimage0.25 has versions 0.25.5–0.25.9 available- No active security advisories (RustSec advisories RUSTSEC-2019-0014 and RUSTSEC-2020-0073 are patched in 0.25+)
- Code uses only stable APIs (
image::open()andto_rgba8()) not affected by the breaking changes in 0.25frontend/src/types/infoDialog.ts (1)
1-1: LGTM!The addition of the 'success' variant to the InfoDialogVariant union is appropriate for providing positive feedback to users (e.g., successful copy-to-clipboard operations).
frontend/src/pages/Home/Home.tsx (2)
27-30: LGTM!The modal state management is clean and follows React best practices. The separation of concerns (open state, current image, and index) is appropriate for this use case.
60-69: LGTM!The modal handlers are well-structured. The cleanup in
handleCloseInfoproperly resets all modal-related state.frontend/src-tauri/src/main.rs (1)
55-58: LGTM with dependencies on path validation fix.The command registration correctly includes both the existing
get_server_pathand the newcopy_image_to_clipboardcommand. Once the path validation is implemented in the command function, this registration is complete.frontend/src/components/Media/ChronologicalGallery.tsx (2)
23-23: LGTM!The addition of the
onViewInfocallback prop is well-typed and follows the existing prop pattern. The signature(image: Image, index: number) => voidprovides all necessary context for the consumer to display image details.
171-172: LGTM!The prop threading is correct. Both
imageIndex(computed from theimageIndexMap) andonViewInfoare properly passed toImageCard, enabling the per-image context menu functionality.frontend/src/components/Media/ImageCard.tsx (2)
102-169: LGTM!The context menu integration is well-structured. The
ContextMenuTriggercorrectly wraps the card, hover state management is preserved, ande.stopPropagation()on the favorite button prevents unintended card clicks.
171-200: LGTM!Context menu actions are well-implemented with proper event propagation handling. The dynamic favorite/unfavorite text and icon styling provide good user feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src-tauri/src/main.rs (1)
29-37: LGTM! Size limit effectively prevents memory exhaustion.The maximum pixel check (30M pixels ≈ 120MB RGBA) prevents excessive memory usage from very large images while allowing typical photos and 4K images. The limit is well-documented and the error message is clear.
Consider making the limit configurable for different deployment environments:
// At file/module level const DEFAULT_MAX_PIXELS: u32 = 30_000_000; // In function, could read from config or use default let max_pixels = DEFAULT_MAX_PIXELS; // Could be read from app config if width * height > max_pixels { return Err(format!( "Image too large to copy to clipboard ({}x{} = {} pixels, max: {})", width, height, width * height, max_pixels ).into()); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src-tauri/src/main.rsfrontend/src-tauri/tauri.conf.jsonfrontend/src/components/Dialog/InfoDialog.tsxfrontend/src/pages/Home/Home.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/Dialog/InfoDialog.tsx
- frontend/src-tauri/tauri.conf.json
- frontend/src/pages/Home/Home.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src-tauri/src/main.rs (2)
backend/app/routes/images.py (1)
ImageData(29-37)frontend/src-tauri/src/services/mod.rs (1)
get_server_path(5-11)
🔇 Additional comments (3)
frontend/src-tauri/src/main.rs (3)
9-16: LGTM! Imports are well-organized.The clipboard-related imports are clearly grouped and all necessary for the command implementation. The organization with comment headers improves code readability.
39-53: LGTM! Clipboard operation is correctly implemented.The image conversion to RGBA8, ImageData construction, and clipboard operation are properly implemented with appropriate error handling. The use of
Cow::Ownedis correct for the owned RGBA data, andarboardhandles cross-platform clipboard differences.
76-79: LGTM! Command registration is correct.The new
copy_image_to_clipboardcommand is properly registered alongside the existingget_server_pathcommand using Tauri'sgenerate_handler!macro.
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @JaYRaNa213. * #900 (comment) The following files were modified: * `frontend/src-tauri/src/main.rs` * `frontend/src/components/Media/ImageCard.tsx`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src-tauri/src/main.rs (2)
9-10: Remove duplicate comment line.The comment "// -------- Clipboard imports --------" appears twice consecutively.
🔎 Proposed fix
-// -------- Clipboard imports -------- // -------- Clipboard imports -------- use tauri::command;
19-65: Add defense-in-depth: canonicalize paths and validate against allowed directories.The current security checks (absolute path requirement, parent dir rejection, file existence validation) provide basic protection against obvious traversal attacks. However, they don't prevent symlink-based attacks.
The gap:
- Symlinks are not resolved: A path like
/home/user/images/link.jpgcan pass all current checks (absolute, no.., exists, is_file) but may be a symlink to/etc/passwdor other sensitive files outside the intended directory.Recommended additions:
- Use
path.canonicalize()to resolve symlinks before validation- Validate the canonicalized path is within allowed base directories (e.g., the app's data directory or user's image library folder)
Enhanced security approach
#[command] -fn copy_image_to_clipboard(path: String) -> Result<(), String> { +fn copy_image_to_clipboard(path: String, app_handle: tauri::AppHandle) -> Result<(), String> { let path = PathBuf::from(&path); // 🔐 Security checks if !path.is_absolute() { return Err("Expected absolute file path".into()); } if path.components().any(|c| matches!(c, std::path::Component::ParentDir)) { return Err("Invalid path traversal detected".into()); } + + // Resolve symlinks and normalize path + let canonical_path = path.canonicalize() + .map_err(|e| format!("Failed to resolve path: {}", e))?; + + // TODO: Define allowed base directories for your app + // Example: validate against app data directory or configured image library path + // let allowed_dirs = vec![ + // app_handle.path().app_data_dir()?, + // PathBuf::from("/path/to/image/library"), + // ]; + // + // if !allowed_dirs.iter().any(|base| canonical_path.starts_with(base)) { + // return Err("Path outside allowed directories".into()); + // } - if !path.exists() { + if !canonical_path.exists() { return Err("File does not exist".into()); } - if !path.is_file() { + if !canonical_path.is_file() { return Err("Path is not a file".into()); } // Load image - let img = image::open(&path) + let img = image::open(&canonical_path) .map_err(|e| format!("Failed to open image: {}", e))?;Note: If you add the
app_handleparameter, update the command invocation in the frontend accordingly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src-tauri/src/main.rs
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src-tauri/src/main.rs (1)
frontend/src-tauri/src/services/mod.rs (1)
get_server_path(5-11)
|
hello |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/app/database/images.py`:
- Around line 473-490: The exception handler in db_add_to_deleted_images
currently logs the error and returns False but does not call conn.rollback();
update the except block in db_add_to_deleted_images to call conn.rollback()
before logging/returning (matching patterns used in db_bulk_insert_images and
db_update_image_tagged_status) so the transaction is rolled back on failure,
then return False; keep the existing finally conn.close() intact.
In `@backend/app/routes/images.py`:
- Around line 138-194: The tombstone insertion return value from
db_add_to_deleted_images(original_path) is ignored in delete_image, so if it
fails the code still deletes files/DB; update delete_image to check the boolean
result of db_add_to_deleted_images(original_path) and abort the flow (raise an
HTTPException with status 500 and a clear detail like "Failed to mark image as
deleted") before deleting files or DB records when the call returns False.
🧹 Nitpick comments (4)
backend/app/utils/images.py (1)
522-552: Function always returnsTrueeven on file deletion failures.The function logs errors when
os.remove()fails but continues and returnsTrue. This means the caller (the delete endpoint) cannot distinguish between successful deletion and partial failures. The route's comment mentions "if this fails, we still proceed with DB cleanup" which suggests this is intentional, but consider whether a partial failure should be surfaced.If this behavior is intentional (best-effort cleanup), consider documenting it in the docstring:
Suggested docstring update
def image_util_delete_image_files(image: dict) -> bool: """ Safely delete original image and its thumbnail from disk. + + This is a best-effort operation: individual file deletion failures are + logged but do not cause the function to return False. Only unexpected + errors return False. Args: image: Image record dictionary containing 'path' and 'thumbnailPath' + + Returns: + True if the operation completed (even with individual file failures), + False only on unexpected errors. """backend/tests/test_image_deletion.py (3)
39-72: Mock image fixture creates non-valid image files.The fixture writes
b"fake image data"which isn't a valid JPEG. While this works for testing the delete endpoint (which doesn't validate image contents), it would fail if any code path callsImage.open()orimage_util_is_valid_image().For more robust tests, consider using a minimal valid JPEG or PNG. However, for the current delete tests, this is acceptable.
96-98: Consider asserting the error response body.The test verifies the 404 status code but doesn't check the response body. Adding an assertion for the error message would improve coverage:
Suggested enhancement
def test_delete_image_not_found(): response = client.delete("/images/non-existent-id") assert response.status_code == 404 + assert response.json()["detail"] == "Image not found"
19-37: Usedb_create_folders_table()instead of manually creating the folders table.The test manually creates a minimal
folderstable, butdb_create_folders_table()is already available in the codebase. While the minimal schema (justfolder_idandfolder_path) is sufficient for the FOREIGN KEY constraint and the tested code paths, using the actual function would eliminate duplication and ensure the test schema stays synchronized with production if the actual schema changes in the future.`@pytest.fixture`(autouse=True) def setup_db(): db_create_images_table() db_create_folders_table() # Use the existing function conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() # Insert a fake folder for FK constraints cursor.execute("INSERT OR IGNORE INTO folders (folder_id, folder_path) VALUES ('1', '/fake/path')") conn.commit() conn.close()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/database/images.pybackend/app/logging/setup_logging.pybackend/app/routes/images.pybackend/app/utils/images.pybackend/tests/test_image_deletion.pydocs/backend/backend_python/openapi.json
🧰 Additional context used
🧬 Code graph analysis (3)
backend/app/utils/images.py (1)
backend/app/database/images.py (1)
db_is_image_deleted(493-503)
backend/tests/test_image_deletion.py (2)
backend/app/database/images.py (4)
db_get_image_by_id(434-470)db_is_image_deleted(493-503)db_bulk_insert_images(99-130)db_create_images_table(52-96)backend/app/utils/images.py (1)
image_util_prepare_image_records(140-186)
backend/app/database/images.py (1)
backend/app/utils/images.py (1)
image_util_parse_metadata(502-519)
🔇 Additional comments (10)
backend/app/logging/setup_logging.py (1)
246-248: Exception tracebacks are silently dropped.The new implementation only forwards the message string via
logging.getLogger().log(record.levelno, ...). Anyexc_infoorstack_infoattached to the original record (e.g., fromlogger.exception()calls in Uvicorn) will be lost, making production error diagnosis difficult.🔧 Proposed fix to preserve exception information
# Create a message that includes the original module in the format msg = record.getMessage() # Use the root logger to avoid recursion if the module-specific logger # is the one we are currently intercepting. - logging.getLogger().log(record.levelno, f"[{module_name}] {msg}") + logging.getLogger().log( + record.levelno, + f"[{module_name}] {msg}", + exc_info=record.exc_info, + stack_info=record.stack_info, + )⛔ Skipped due to learnings
Learnt from: Hemil36 Repo: AOSSIE-Org/PictoPy PR: 548 File: backend/app/logging/setup_logging.py:224-243 Timestamp: 2025-10-13T16:40:22.622Z Learning: In the PictoPy project's centralized logging system (backend/app/logging/setup_logging.py and sync-microservice/app/logging/setup_logging.py), stack traces should not be shown in logs. When rerouting logs through InterceptHandler, exc_info and stack_info should not be preserved.Learnt from: Hemil36 Repo: AOSSIE-Org/PictoPy PR: 548 File: backend/app/database/images.py:115-115 Timestamp: 2025-10-15T07:13:34.502Z Learning: In the PictoPy backend and sync-microservice, exception logging using `logger.error(f"...")` without stack traces (i.e., without `exc_info=True` or `logger.exception()`) is acceptable for console logging, as this is a deliberate design decision made with the maintainer for console-only logs.backend/app/database/images.py (3)
85-93: Tombstone table design looks good.The
deleted_imagestable correctly usespathas the primary key and includes adeleted_attimestamp for audit purposes. UsingCREATE TABLE IF NOT EXISTSensures idempotent schema creation.
434-470: LGTM fordb_get_image_by_id.The function correctly fetches a single image record, handles the
Nonecase when the image is not found, and properly parses metadata using the existing utility. The structure is consistent with other functions in this module.
493-502: LGTM fordb_is_image_deleted.The tombstone check function is simple and correct. Using
SELECT 1is efficient for existence checks.backend/app/utils/images.py (1)
155-158: Good tombstone integration.The tombstone check correctly prevents re-indexing of previously deleted images. Using
db_is_image_deletedbefore processing ensures deleted files won't reappear in the library after manual restoration.backend/app/routes/images.py (1)
3-11: Imports are well-organized.The expanded imports properly bring in all necessary database functions, schema types, and utilities for the delete functionality.
docs/backend/backend_python/openapi.json (2)
929-991: OpenAPI spec accurately documents the new DELETE endpoint.The path documentation includes:
- Correct HTTP method and path parameter
- Comprehensive description of the 4-step deletion process
- Proper response schemas for success (200), not found (404), validation error (422), and server error (500)
1685-1707: DeleteImageResponse schema is consistent with the implementation.The schema correctly defines
data,message, andsuccessas required fields with appropriate types.backend/tests/test_image_deletion.py (2)
74-94: Comprehensive test for successful deletion.The test properly verifies:
- Preconditions (files and DB record exist)
- API response (200 status, success=true)
- Postconditions (files deleted, DB record removed, tombstone created)
100-121: Good test for tombstone preventing re-indexing.This test correctly validates the core tombstone behavior:
- Delete image (creates tombstone)
- Recreate file on disk
- Verify
image_util_prepare_image_recordsskips the tombstoned pathMinor note: The inline import on line 114 could be moved to the top of the file for consistency.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def db_add_to_deleted_images(path: str) -> bool: | ||
| """ | ||
| Record a path in the deleted_images tombstone table. | ||
| """ | ||
| conn = _connect() | ||
| cursor = conn.cursor() | ||
| try: | ||
| cursor.execute( | ||
| "INSERT OR IGNORE INTO deleted_images (path) VALUES (?)", | ||
| (path,), | ||
| ) | ||
| conn.commit() | ||
| return True | ||
| except Exception as e: | ||
| logger.error(f"Error adding to deleted_images: {e}") | ||
| return False | ||
| finally: | ||
| conn.close() |
There was a problem hiding this comment.
Missing conn.rollback() before returning False on exception.
For consistency with other database functions in this file (e.g., db_bulk_insert_images, db_update_image_tagged_status), the exception handler should call conn.rollback() before returning False.
Proposed fix
except Exception as e:
logger.error(f"Error adding to deleted_images: {e}")
+ conn.rollback()
return False
finally:
conn.close()🤖 Prompt for AI Agents
In `@backend/app/database/images.py` around lines 473 - 490, The exception handler
in db_add_to_deleted_images currently logs the error and returns False but does
not call conn.rollback(); update the except block in db_add_to_deleted_images to
call conn.rollback() before logging/returning (matching patterns used in
db_bulk_insert_images and db_update_image_tagged_status) so the transaction is
rolled back on failure, then return False; keep the existing finally
conn.close() intact.
| @router.delete( | ||
| "/{image_id}", | ||
| response_model=DeleteImageResponse, | ||
| responses={404: {"model": ErrorResponse}, 500: {"model": ErrorResponse}}, | ||
| ) | ||
| def delete_image(image_id: str): | ||
| """ | ||
| Explicitly delete an image from the library. | ||
| Steps: | ||
| 1. Fetch image record from DB. | ||
| 2. Add to 'deleted_images' tombstone (prevents re-sync). | ||
| 3. Delete physical files (original + thumbnail). | ||
| 4. Delete DB record (triggers cascaded delete for tags/metadata). | ||
| """ | ||
| try: | ||
| logger.info(f"Image deletion requested for ID: {image_id}") | ||
|
|
||
| # 1. Fetch image record | ||
| image = db_get_image_by_id(image_id) | ||
| if not image: | ||
| logger.warning(f"Image deletion failed: ID {image_id} not found") | ||
| raise HTTPException(status_code=404, detail="Image not found") | ||
|
|
||
| original_path = image.get("path") | ||
|
|
||
| # 2. Add to tombstone | ||
| db_add_to_deleted_images(original_path) | ||
|
|
||
| # 3. Delete files | ||
| # We do this before DB deletion to have the paths available, | ||
| # but if this fails, we still proceed with DB cleanup to keep it consistent. | ||
| image_util_delete_image_files(image) | ||
|
|
||
| # 4. Delete DB record | ||
| # This will trigger ON DELETE CASCADE for faces, album_images, image_classes | ||
| db_delete_images_by_ids([image_id]) | ||
|
|
||
| logger.info(f"Successfully deleted image {image_id} and path {original_path}") | ||
|
|
||
| return DeleteImageResponse( | ||
| success=True, | ||
| message="Image deleted successfully", | ||
| data=image_id, | ||
| ) | ||
|
|
||
| except HTTPException: | ||
| raise | ||
| except Exception as e: | ||
| logger.error(f"Error deleting image {image_id}: {e}") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=ErrorResponse( | ||
| success=False, | ||
| error="Internal server error", | ||
| message=f"Failed to delete image: {str(e)}", | ||
| ).model_dump(), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the implementation of db_add_to_deleted_images
rg -n "def db_add_to_deleted_images" --type=py -A 10Repository: AOSSIE-Org/PictoPy
Length of output: 751
🏁 Script executed:
# Check all usages of db_add_to_deleted_images to see if return value is checked
rg -n "db_add_to_deleted_images" --type=py -B 2 -A 2Repository: AOSSIE-Org/PictoPy
Length of output: 936
🏁 Script executed:
# Look for similar tombstone/deletion patterns in the codebase
rg -n "deleted_images|tombstone" --type=py -A 3 -B 1Repository: AOSSIE-Org/PictoPy
Length of output: 4871
🏁 Script executed:
# Get full implementation of db_add_to_deleted_images to confirm return value handling
sed -n '473,491p' backend/app/database/images.pyRepository: AOSSIE-Org/PictoPy
Length of output: 552
🏁 Script executed:
# Verify the delete_image endpoint doesn't check the return value
sed -n '138,194p' backend/app/routes/images.py | grep -A 30 "Add to tombstone"Repository: AOSSIE-Org/PictoPy
Length of output: 1186
Check return value of db_add_to_deleted_images() and abort deletion if tombstone insertion fails.
The function returns bool (True on success, False on exception), but the return value is ignored in the delete_image endpoint at line 164:
db_add_to_deleted_images(original_path) # Return value not checkedIf tombstone insertion fails, deletion proceeds anyway, leaving the image unprotected against re-indexing on the next sync. This violates the tombstone's safety guarantee.
Suggested fix:
if not db_add_to_deleted_images(original_path):
raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail="Failed to mark image as deleted"
)🤖 Prompt for AI Agents
In `@backend/app/routes/images.py` around lines 138 - 194, The tombstone insertion
return value from db_add_to_deleted_images(original_path) is ignored in
delete_image, so if it fails the code still deletes files/DB; update
delete_image to check the boolean result of
db_add_to_deleted_images(original_path) and abort the flow (raise an
HTTPException with status 500 and a clear detail like "Failed to mark image as
deleted") before deleting files or DB records when the call returns False.
1 participant
This PR introduces a right-click context menu for image cards in PictoPy, enabling quick and intuitive image actions without affecting existing workflows.
The feature improves usability by allowing users to interact directly with images from the gallery view.
Issue :
Fixes #802
✨ Features Added
🖱️ Right-click Context Menu on Image Cards
📁 Files Changed
Frontend
Backend (Tauri)
Screenshots
View Info button :
Screenshots
- >
Summary by CodeRabbit
New Features
New Backend
UI/UX Improvements
Tests & Docs
✏️ Tip: You can customize this high-level summary in your review settings.