Right-Click Context Menu Actions for Images *(Favorite, Copy, View Details)* #1043
Conversation
📝 WalkthroughWalkthroughThis pull request implements right-click context menu functionality for images alongside a complete backend image deletion feature. It introduces a tombstone-based deletion mechanism in the database, a new API endpoint for image deletion with file cleanup, and a context menu UI with copy-to-clipboard, view info, and deletion actions. Frontend enhancements include Tauri clipboard support and integrated media info viewing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImageCard
participant Tauri as Tauri Command
participant System as System Clipboard
participant Redux
participant InfoDialog
User->>ImageCard: Right-click on image
ImageCard->>ImageCard: Show context menu
User->>ImageCard: Click "Copy Image"
ImageCard->>Tauri: invoke('copy_image_to_clipboard', path)
Tauri->>Tauri: Validate path, load image
Tauri->>Tauri: Convert to RGBA
Tauri->>System: Write to clipboard
alt Success
Tauri-->>ImageCard: Ok(())
ImageCard->>Redux: dispatch(showInfoDialog('success'))
Redux->>InfoDialog: Display success message
else Error
Tauri-->>ImageCard: Err(message)
ImageCard->>Redux: dispatch(showInfoDialog('error'))
Redux->>InfoDialog: Display error message
end
sequenceDiagram
participant User
participant ImageCard
participant API as Backend API
participant Database
participant FileSystem
participant Redux
participant InfoDialog
User->>ImageCard: Right-click, click "Delete"
ImageCard->>API: DELETE /images/{image_id}
alt Image Found
API->>Database: db_get_image_by_id(image_id)
Database-->>API: image record
API->>Database: db_add_to_deleted_images(path)
Database-->>API: acknowledged
API->>FileSystem: Delete image & thumbnail files
FileSystem-->>API: files deleted
API->>Database: db_delete_images_by_ids(image_id)
Database-->>API: record deleted
API-->>ImageCard: 200 DeleteImageResponse
ImageCard->>Redux: dispatch(showInfoDialog('success'))
else Image Not Found
API-->>ImageCard: 404 ErrorResponse
ImageCard->>Redux: dispatch(showInfoDialog('error'))
end
Redux->>InfoDialog: Display response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In `@backend/app/database/images.py`:
- Around line 434-470: db_get_image_by_id currently converts folder_id with
str(folder_id) which yields the string "None" when folder_id is NULL; update
db_get_image_by_id to mirror db_get_all_images by returning None for folder_id
when the DB value is NULL (e.g., set "folder_id": str(folder_id) if folder_id is
not None else None) so downstream consumers get a true None instead of the
string "None".
In `@backend/app/routes/images.py`:
- Around line 161-173: The code currently reads original_path =
image.get("path") and passes it to db_add_to_deleted_images without validation;
update the block to validate original_path (non-None and non-empty) before
calling db_add_to_deleted_images so you don't insert NULL/empty paths into the
tombstone table—if original_path is missing, skip the tombstone insert and
optionally log or record a warning; still proceed with
image_util_delete_image_files(image) and db_delete_images_by_ids([image_id]) as
before.
In `@backend/tests/test_image_deletion.py`:
- Around line 44-51: The test writes non-image bytes which causes
PIL.Image.open() in image_util_generate_thumbnail (called by
image_util_prepare_image_records) to fail and masks the real tombstone behavior;
replace the raw b"fake image data" writes in the fixture used by
test_tombstone_prevents_indexing with a minimal valid JPEG/PNG byte payload
(create a 1x1 image via PIL.Image.new and save it into an in-memory buffer, then
write buffer.getvalue() to both img_path and thumb_path) and add the necessary
PIL import to the test so thumbnail generation succeeds and the tombstone logic
is actually exercised.
In `@frontend/src-tauri/tauri.conf.json`:
- Around line 69-71: The assetProtocol.scope value currently set to ["**"] is
overly permissive; update the tauri.conf.json assetProtocol.scope to restrict
paths to only required directories (e.g., use predefined variables like
$PICTURES, $DOWNLOADS, $DOCUMENTS or explicit user-selected folders) or switch
to Tauri's persisted-scope plugin to grant runtime-managed access; if broad
patterns are absolutely required, add explicit deny entries for sensitive system
and hidden config paths and ensure any backend code that validates scope uses
the tightened scope values instead of assuming global access.
🧹 Nitpick comments (11)
frontend/src-tauri/Cargo.toml (1)
19-20: Consider updating arboard to the latest version; image 0.25 breaking changes require verification.The
arboardcrate is appropriate for clipboard operations, but version 3.4 is outdated—the latest stable release is 3.6.1. Theimagecrate upgrade to 0.25 involves substantial breaking changes including decoder signature changes (now requiringBufRead + Seek), API replacements (ColorType→ExtendedColorType), and removed APIs likeGenericImageView::bounds). The current usage ofimage::open()andGenericImageViewshould be tested to ensure compatibility with these changes. If the code compiles and clipboard operations work correctly, these dependencies are acceptable, but consider upgrading arboard to 3.6.1 for better maintenance.frontend/src/components/ui/context-menu.tsx (1)
1-7: Minor: Empty line at file start.Line 1 is blank. While not a functional issue, it's inconsistent with typical file formatting.
Suggested fix
- import * as React from "react"docs/backend/backend_python/openapi.json (1)
1685-1707: Consider ifdatafield should be optional in DeleteImageResponse.The
datafield is marked as required and typed asstring. For delete operations, typically:
datamight be empty or contain the deleted resource's ID- Making it optional (
anyOfwithnull) would align with other response schemas likeDeleteFoldersResponseThis is a minor inconsistency with other schemas in the API that use nullable
datafields.Suggested schema alignment
"DeleteImageResponse": { "properties": { "data": { - "type": "string", + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], "title": "Data" }, ... }, "required": [ - "data", "message", "success" ],backend/app/utils/images.py (1)
24-30: Duplicate logger initialization.The logger is initialized twice: once using
get_logger(__name__)on line 24 and again usinglogging.getLogger(__name__)on line 30. The second declaration shadows the first.♻️ Suggested fix
logger = get_logger(__name__) # GPS EXIF tag constant GPS_INFO_TAG = 34853 - -logger = logging.getLogger(__name__)backend/tests/test_image_deletion.py (1)
28-35: Potential type mismatch forfolder_id.The
folder_idis inserted as a string'1'in the folders table, but the image record at line 56 uses an integer1. This may work due to SQLite's type affinity, but it's inconsistent. Consider aligning the types for clarity.♻️ Suggested fix
cursor.execute(""" CREATE TABLE IF NOT EXISTS folders ( - folder_id TEXT PRIMARY KEY, + folder_id INTEGER PRIMARY KEY, folder_path TEXT UNIQUE ) """) # Insert a fake folder for FK constraints - cursor.execute("INSERT OR IGNORE INTO folders (folder_id, folder_path) VALUES ('1', '/fake/path')") + cursor.execute("INSERT OR IGNORE INTO folders (folder_id, folder_path) VALUES (1, '/fake/path')")frontend/src-tauri/src/main.rs (2)
9-10: Duplicate comment.Lines 9 and 10 contain the same comment.
♻️ Suggested fix
// -------- Clipboard imports -------- -// -------- Clipboard imports -------- use tauri::command;
19-65: Consider usingcanonicalize()for more robust path validation.The current path traversal check using
Component::ParentDirmay reject legitimate absolute paths that happen to contain..segments before being passed in, even if they resolve to valid locations. Usingcanonicalize()would normalize the path and also verify the file exists in one step.♻️ Suggested improvement
#[command] fn copy_image_to_clipboard(path: String) -> 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()); - } - - if !path.exists() { - return Err("File does not exist".into()); - } - - if !path.is_file() { - return Err("Path is not a file".into()); - } + // 🔐 Security checks - canonicalize resolves symlinks and ".." components + let canonical_path = path.canonicalize() + .map_err(|_| "File does not exist or path is invalid")?; + + 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:
canonicalize()follows symlinks. If symlink traversal is a security concern for your use case, consider adding additional validation after canonicalization to ensure the resolved path is within an allowed directory.frontend/src/pages/Home/Home.tsx (1)
101-115: Consider adding keyboard accessibility for closing the modal.The modal can be closed by clicking the backdrop or the close button inside
MediaInfoPanel, but there's no keyboard handler for the Escape key. This is an accessibility best practice for modal dialogs.♻️ Suggested enhancement
Add an effect to handle Escape key:
useEffect(() => { if (!isInfoOpen) return; const handleKeyDown = (e: KeyboardEvent) => { if (e.key === 'Escape') { handleCloseInfo(); } }; document.addEventListener('keydown', handleKeyDown); return () => document.removeEventListener('keydown', handleKeyDown); }, [isInfoOpen]);backend/app/database/images.py (1)
473-490: Addconn.rollback()on exception for consistency.Other database functions in this file consistently call
conn.rollback()when an exception occurs (e.g., lines 127, 297, 331, 403, 428). Although SQLite auto-rolls back uncommitted transactions on connection close, adding explicit rollback maintains consistency with the codebase patterns.Proposed fix
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}") + conn.rollback() return False finally: conn.close()frontend/src/components/Media/ImageCard.tsx (1)
65-88: Add validation forimage.pathbefore invoking clipboard copy.If
image.pathisundefinedor empty, the Tauri invoke call may fail unexpectedly. Adding a guard improves error messaging and prevents unnecessary backend calls.Proposed fix
const handleCopy = async () => { try { + if (!image.path) { + dispatch( + showInfoDialog({ + title: 'Error', + message: 'Image path not available', + variant: 'error', + }), + ); + return; + } await invoke('copy_image_to_clipboard', { path: image.path, });backend/app/routes/images.py (1)
163-173: Add error handling and wrap critical operations in a single transaction.The deletion workflow lacks error checking for all three operations and uses separate transactions. If
db_delete_images_by_idsfails afterdb_add_to_deleted_imagescommits, the system enters an inconsistent state (tombstone exists, files deleted, but DB record remains). Additionally, the return values of all three operations are ignored, so failures silently propagate to the generic exception handler.Refactor to:
- Check return values of
db_add_to_deleted_images,image_util_delete_image_files, anddb_delete_images_by_idsbefore proceeding- Wrap the tombstone insertion and DB deletion in a single transaction to ensure atomicity
- Implement compensation logic (e.g., remove tombstone) if the final DB delete fails
📜 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 (18)
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.jsonfrontend/package.jsonfrontend/src-tauri/Cargo.tomlfrontend/src-tauri/src/main.rsfrontend/src-tauri/tauri.conf.jsonfrontend/src/App.tsxfrontend/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
🧠 Learnings (1)
📚 Learning: 2025-10-13T16:40:22.622Z
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.
Applied to files:
backend/app/logging/setup_logging.py
🧬 Code graph analysis (8)
backend/app/routes/images.py (3)
backend/app/database/images.py (3)
db_toggle_image_favourite_status(409-431)db_get_image_by_id(434-470)db_delete_images_by_ids(374-406)backend/app/schemas/images.py (1)
DeleteImageResponse(71-74)backend/app/utils/images.py (2)
image_util_parse_metadata(502-519)image_util_delete_image_files(522-552)
frontend/src-tauri/src/main.rs (1)
frontend/src-tauri/src/services/mod.rs (1)
get_resources_folder_path(5-11)
frontend/src/components/Media/ChronologicalGallery.tsx (1)
frontend/src/types/Media.ts (1)
Image(13-23)
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)
backend/app/database/images.py (3)
backend/app/utils/images.py (1)
image_util_parse_metadata(502-519)backend/app/models/FaceDetector.py (1)
close(69-79)backend/app/models/FaceNet.py (1)
close(27-29)
frontend/src/pages/Home/Home.tsx (2)
frontend/src/types/Media.ts (1)
Image(13-23)frontend/src/components/Media/MediaInfoPanel.tsx (1)
MediaInfoPanel(24-182)
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/utils/images.py (1)
backend/app/database/images.py (1)
db_is_image_deleted(493-503)
🔇 Additional comments (29)
frontend/src/App.tsx (1)
11-11: LGTM!Whitespace-only change; no functional impact.
frontend/src/types/infoDialog.ts (1)
1-1: LGTM!The
'success'variant addition is appropriate for confirming successful operations like copy-to-clipboard. The change is backward compatible with existing usages.frontend/src/components/Dialog/InfoDialog.tsx (2)
1-1: LGTM!Clean import addition for the new success variant icon.
42-47: Well-structured success variant implementation.The configuration follows the established pattern for
infoanderrorvariants. The green color is semantically appropriate, and the emptymessageColorcorrectly defaults to the standard text color.frontend/package.json (1)
28-28: Appropriate dependency addition for context menu functionality.The
@radix-ui/react-context-menupackage aligns with the existing Radix UI ecosystem in this project and supports the new right-click context menu feature. Version ^2.2.16 is the latest available.backend/app/logging/setup_logging.py (1)
246-248: LGTM! Simplified log forwarding avoids recursion.The refactored
emitmethod correctly uses the root logger directly vialogging.getLogger().log(), which prevents recursion when the module-specific logger is being intercepted. The module name prefix in the message provides traceability. Based on learnings, this approach correctly avoids preservingexc_infoandstack_info.frontend/src/components/Media/MediaInfoPanel.tsx (2)
13-13: LGTM! Clean addition of className prop for external styling.The
classNameprop is properly typed as optional, destructured, and applied via thecnutility for Tailwind class merging. This allows parent components to customize the panel's positioning or appearance.Also applies to: 21-21, 30-30, 67-67
111-124: LGTM! Location rendering logic is correct.The condition properly checks for both
latitudeANDlongitudebefore rendering the clickable map link, with a fallback message when either is unavailable. ThehandleLocationClickfunction (lines 52-62) has consistent guards.frontend/src/components/ui/context-menu.tsx (3)
8-89: LGTM! Well-structured Radix UI Context Menu wrappers.The component implementations follow the standard shadcn/ui pattern:
- Proper ref forwarding with
React.forwardRef- Consistent use of
cnfor class merging- Correct
displayNameassignments for debugging- Appropriate use of
insetprop for alignment variations
91-181: LGTM! Checkbox, Radio, Label, Separator, and Shortcut components are correctly implemented.
ContextMenuCheckboxItemandContextMenuRadioItemproperly render indicators with Lucide iconsContextMenuShortcutis a simple styled span without ref forwarding, which is appropriate for its use case
183-199: LGTM! Complete export list.All components are properly exported for external consumption.
frontend/src-tauri/tauri.conf.json (1)
49-51: LGTM! Clipboard plugin enabled for native clipboard support.This configuration enables the Tauri clipboard plugin, which is required for the
copy_image_to_clipboardcommand to function in the desktop app.docs/backend/backend_python/openapi.json (1)
929-991: LGTM! Delete image endpoint is well-documented.The endpoint follows RESTful conventions with proper path parameter, comprehensive response codes (200, 404, 500, 422), and a clear description of the tombstone-based deletion flow.
backend/app/utils/images.py (2)
18-18: LGTM!Import added correctly to support the tombstone check functionality.
155-158: LGTM!Tombstone check correctly prevents re-indexing of previously deleted images, preserving user intent even if the file is recreated.
backend/tests/test_image_deletion.py (2)
74-94: LGTM!Good test coverage for the successful deletion flow: verifies file existence before deletion, confirms API response, validates file removal and database state, and checks tombstone creation.
100-121: Strengthen tombstone verification; clarify path absoluteness.The test already uses absolute paths via
tmp_path, but for defensive robustness, explicitly convertfolder_pathto absolute usingos.path.abspath(). More importantly, the assertionassert len(records) == 0could pass due to thumbnail generation failure (the recreated fileb"recreated file"is not a valid image) rather than due to the tombstone check. Add an explicit assertion confirming the tombstone check was the reason for skipping, such as verifying the log output or mockingdb_is_image_deletedto confirm it returnsTruefor the recreated image path.frontend/src/components/Media/ChronologicalGallery.tsx (2)
23-23: LGTM!The
onViewInfoprop is correctly typed as optional and follows the established pattern for image-related callbacks in the codebase.
169-177: LGTM!Clean prop forwarding to
ImageCardwith the computedchronologicalIndexenabling consistent index tracking across the gallery.frontend/src-tauri/src/main.rs (1)
88-91: LGTM!Command properly registered in the invoke handler alongside existing services.
frontend/src/pages/Home/Home.tsx (2)
27-30: LGTM!State management for the info modal is cleanly separated with appropriate types.
60-69: LGTM!Handler functions are concise and correctly manage modal state. Clearing
infoImageon close prevents stale data display.backend/app/database/images.py (2)
85-93: LGTM! Tombstone table for tracking deleted images.The tombstone pattern is appropriate for preventing re-sync of deleted images. The schema correctly uses
pathas the primary key and includes a timestamp for auditing purposes.
493-502: LGTM! Simple and correct tombstone lookup.The function properly checks existence in the tombstone table and handles the connection lifecycle correctly.
backend/app/routes/images.py (2)
3-11: LGTM! Import additions are appropriate.The new imports align well with the deletion functionality being added.
177-181: LGTM! Response structure is correct.The
DeleteImageResponsecorrectly includessuccess,message, anddatafields matching the schema definition.frontend/src/components/Media/ImageCard.tsx (3)
5-29: LGTM! Import organization is clean.The new imports for context menu components, icons, Redux dispatch, and Tauri invoke are well-organized and appropriate for the new functionality.
94-101: LGTM! Graceful fallback forhandleViewInfo.The fallback to
onClickwhenonViewInfois not provided maintains backward compatibility with existing usages.
172-201: LGTM! Context menu implementation is correct.The context menu properly:
- Stops event propagation to prevent triggering the card's
onClick- Provides clear labels and appropriate icons
- Uses a separator to visually group related actions
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def db_get_image_by_id(image_id: str) -> Optional[dict]: | ||
| """ | ||
| Fetch a single image record by its ID. | ||
| """ | ||
| conn = _connect() | ||
| cursor = conn.cursor() | ||
| try: | ||
| cursor.execute( | ||
| "SELECT id, path, folder_id, thumbnailPath, metadata, isTagged, isFavourite FROM images WHERE id = ?", | ||
| (image_id,), | ||
| ) | ||
| row = cursor.fetchone() | ||
| if not row: | ||
| return None | ||
|
|
||
| from app.utils.images import image_util_parse_metadata | ||
|
|
||
| ( | ||
| img_id, | ||
| path, | ||
| folder_id, | ||
| thumbnail_path, | ||
| metadata, | ||
| is_tagged, | ||
| is_favourite, | ||
| ) = row | ||
| return { | ||
| "id": img_id, | ||
| "path": path, | ||
| "folder_id": str(folder_id), | ||
| "thumbnailPath": thumbnail_path, | ||
| "metadata": image_util_parse_metadata(metadata), | ||
| "isTagged": bool(is_tagged), | ||
| "isFavourite": bool(is_favourite), | ||
| } | ||
| finally: | ||
| conn.close() |
There was a problem hiding this comment.
Handle potential None value for folder_id.
If folder_id is None in the database, str(folder_id) will return the string "None" rather than None, which may cause downstream issues or type inconsistencies with other functions like db_get_all_images (line 262) that handle this case explicitly.
Proposed fix
return {
"id": img_id,
"path": path,
- "folder_id": str(folder_id),
+ "folder_id": str(folder_id) if folder_id is not None else None,
"thumbnailPath": thumbnail_path,
"metadata": image_util_parse_metadata(metadata),
"isTagged": bool(is_tagged),
"isFavourite": bool(is_favourite),
}📝 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.
| def db_get_image_by_id(image_id: str) -> Optional[dict]: | |
| """ | |
| Fetch a single image record by its ID. | |
| """ | |
| conn = _connect() | |
| cursor = conn.cursor() | |
| try: | |
| cursor.execute( | |
| "SELECT id, path, folder_id, thumbnailPath, metadata, isTagged, isFavourite FROM images WHERE id = ?", | |
| (image_id,), | |
| ) | |
| row = cursor.fetchone() | |
| if not row: | |
| return None | |
| from app.utils.images import image_util_parse_metadata | |
| ( | |
| img_id, | |
| path, | |
| folder_id, | |
| thumbnail_path, | |
| metadata, | |
| is_tagged, | |
| is_favourite, | |
| ) = row | |
| return { | |
| "id": img_id, | |
| "path": path, | |
| "folder_id": str(folder_id), | |
| "thumbnailPath": thumbnail_path, | |
| "metadata": image_util_parse_metadata(metadata), | |
| "isTagged": bool(is_tagged), | |
| "isFavourite": bool(is_favourite), | |
| } | |
| finally: | |
| conn.close() | |
| def db_get_image_by_id(image_id: str) -> Optional[dict]: | |
| """ | |
| Fetch a single image record by its ID. | |
| """ | |
| conn = _connect() | |
| cursor = conn.cursor() | |
| try: | |
| cursor.execute( | |
| "SELECT id, path, folder_id, thumbnailPath, metadata, isTagged, isFavourite FROM images WHERE id = ?", | |
| (image_id,), | |
| ) | |
| row = cursor.fetchone() | |
| if not row: | |
| return None | |
| from app.utils.images import image_util_parse_metadata | |
| ( | |
| img_id, | |
| path, | |
| folder_id, | |
| thumbnail_path, | |
| metadata, | |
| is_tagged, | |
| is_favourite, | |
| ) = row | |
| return { | |
| "id": img_id, | |
| "path": path, | |
| "folder_id": str(folder_id) if folder_id is not None else None, | |
| "thumbnailPath": thumbnail_path, | |
| "metadata": image_util_parse_metadata(metadata), | |
| "isTagged": bool(is_tagged), | |
| "isFavourite": bool(is_favourite), | |
| } | |
| finally: | |
| conn.close() |
🤖 Prompt for AI Agents
In `@backend/app/database/images.py` around lines 434 - 470, db_get_image_by_id
currently converts folder_id with str(folder_id) which yields the string "None"
when folder_id is NULL; update db_get_image_by_id to mirror db_get_all_images by
returning None for folder_id when the DB value is NULL (e.g., set "folder_id":
str(folder_id) if folder_id is not None else None) so downstream consumers get a
true None instead of the string "None".
| 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]) |
There was a problem hiding this comment.
Validate original_path before tombstoning.
If image.get("path") returns None, passing it to db_add_to_deleted_images will insert a NULL path into the tombstone table, which could cause issues with future lookups or constraint violations.
Proposed fix
original_path = image.get("path")
+ if not original_path:
+ logger.error(f"Image {image_id} has no path, cannot delete")
+ raise HTTPException(status_code=500, detail="Image has no path")
# 2. Add to tombstone
db_add_to_deleted_images(original_path)📝 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.
| 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]) | |
| original_path = image.get("path") | |
| if not original_path: | |
| logger.error(f"Image {image_id} has no path, cannot delete") | |
| raise HTTPException(status_code=500, detail="Image has no 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]) |
🤖 Prompt for AI Agents
In `@backend/app/routes/images.py` around lines 161 - 173, The code currently
reads original_path = image.get("path") and passes it to
db_add_to_deleted_images without validation; update the block to validate
original_path (non-None and non-empty) before calling db_add_to_deleted_images
so you don't insert NULL/empty paths into the tombstone table—if original_path
is missing, skip the tombstone insert and optionally log or record a warning;
still proceed with image_util_delete_image_files(image) and
db_delete_images_by_ids([image_id]) as before.
| def image_util_delete_image_files(image: dict) -> bool: | ||
| """ | ||
| Safely delete original image and its thumbnail from disk. | ||
| Args: | ||
| image: Image record dictionary containing 'path' and 'thumbnailPath' | ||
| """ | ||
| try: | ||
| original_path = image.get("path") | ||
| thumbnail_path = image.get("thumbnailPath") | ||
|
|
||
| # Delete original file | ||
| if original_path and os.path.exists(original_path): | ||
| try: | ||
| os.remove(original_path) | ||
| logger.info(f"Deleted original image: {original_path}") | ||
| except OSError as e: | ||
| logger.error(f"Error deleting original image {original_path}: {e}") | ||
|
|
||
| # Delete thumbnail | ||
| if thumbnail_path and os.path.exists(thumbnail_path): | ||
| try: | ||
| os.remove(thumbnail_path) | ||
| logger.info(f"Deleted thumbnail: {thumbnail_path}") | ||
| except OSError as e: | ||
| logger.error(f"Error deleting thumbnail {thumbnail_path}: {e}") | ||
|
|
||
| return True | ||
| except Exception as e: | ||
| logger.error(f"Unexpected error in image_util_delete_image_files: {e}") | ||
| return False |
There was a problem hiding this comment.
Function always returns True even when file deletion fails.
The function logs errors when os.remove() fails but continues and returns True. This could mask partial failures where the original image is deleted but the thumbnail deletion fails (or vice versa). The docstring also has an incomplete "Returns" section.
🐛 Proposed fix to track deletion success
def image_util_delete_image_files(image: dict) -> bool:
"""
Safely delete original image and its thumbnail from disk.
Args:
image: Image record dictionary containing 'path' and 'thumbnailPath'
+
+ Returns:
+ bool: True if all deletions succeeded, False on any error
"""
try:
original_path = image.get("path")
thumbnail_path = image.get("thumbnailPath")
+ success = True
# Delete original file
if original_path and os.path.exists(original_path):
try:
os.remove(original_path)
logger.info(f"Deleted original image: {original_path}")
except OSError as e:
logger.error(f"Error deleting original image {original_path}: {e}")
+ success = False
# Delete thumbnail
if thumbnail_path and os.path.exists(thumbnail_path):
try:
os.remove(thumbnail_path)
logger.info(f"Deleted thumbnail: {thumbnail_path}")
except OSError as e:
logger.error(f"Error deleting thumbnail {thumbnail_path}: {e}")
+ success = False
- return True
+ return success
except Exception as e:
logger.error(f"Unexpected error in image_util_delete_image_files: {e}")
return False| img_path = img_dir / "test_image.jpg" | ||
| img_path.write_bytes(b"fake image data") | ||
|
|
||
| thumb_dir = tmp_path / "thumbnails" | ||
| thumb_dir.mkdir() | ||
| thumb_path = thumb_dir / "test_thumb.jpg" | ||
| thumb_path.write_bytes(b"fake thumb data") | ||
|
|
There was a problem hiding this comment.
Fake image data may cause test failures.
The test writes b"fake image data" which is not valid image data. The image_util_prepare_image_records function calls image_util_generate_thumbnail, which uses PIL.Image.open(). This will fail for non-image files. The test_tombstone_prevents_indexing test may incorrectly pass because thumbnail generation fails, not because of the tombstone check.
🐛 Proposed fix using a minimal valid JPEG
+# Minimal valid 1x1 JPEG (red pixel)
+MINIMAL_JPEG = bytes([
+ 0xFF, 0xD8, 0xFF, 0xE0, 0x00, 0x10, 0x4A, 0x46, 0x49, 0x46, 0x00, 0x01,
+ 0x01, 0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00, 0xFF, 0xDB, 0x00, 0x43,
+ 0x00, 0x08, 0x06, 0x06, 0x07, 0x06, 0x05, 0x08, 0x07, 0x07, 0x07, 0x09,
+ 0x09, 0x08, 0x0A, 0x0C, 0x14, 0x0D, 0x0C, 0x0B, 0x0B, 0x0C, 0x19, 0x12,
+ 0x13, 0x0F, 0x14, 0x1D, 0x1A, 0x1F, 0x1E, 0x1D, 0x1A, 0x1C, 0x1C, 0x20,
+ 0x24, 0x2E, 0x27, 0x20, 0x22, 0x2C, 0x23, 0x1C, 0x1C, 0x28, 0x37, 0x29,
+ 0x2C, 0x30, 0x31, 0x34, 0x34, 0x34, 0x1F, 0x27, 0x39, 0x3D, 0x38, 0x32,
+ 0x3C, 0x2E, 0x33, 0x34, 0x32, 0xFF, 0xC0, 0x00, 0x0B, 0x08, 0x00, 0x01,
+ 0x00, 0x01, 0x01, 0x01, 0x11, 0x00, 0xFF, 0xC4, 0x00, 0x1F, 0x00, 0x00,
+ 0x01, 0x05, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
+ 0x09, 0x0A, 0x0B, 0xFF, 0xC4, 0x00, 0xB5, 0x10, 0x00, 0x02, 0x01, 0x03,
+ 0x03, 0x02, 0x04, 0x03, 0x05, 0x05, 0x04, 0x04, 0x00, 0x00, 0x01, 0x7D,
+ 0x01, 0x02, 0x03, 0x00, 0x04, 0x11, 0x05, 0x12, 0x21, 0x31, 0x41, 0x06,
+ 0x13, 0x51, 0x61, 0x07, 0x22, 0x71, 0x14, 0x32, 0x81, 0x91, 0xA1, 0x08,
+ 0x23, 0x42, 0xB1, 0xC1, 0x15, 0x52, 0xD1, 0xF0, 0x24, 0x33, 0x62, 0x72,
+ 0x82, 0x09, 0x0A, 0x16, 0x17, 0x18, 0x19, 0x1A, 0x25, 0x26, 0x27, 0x28,
+ 0x29, 0x2A, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x3A, 0x43, 0x44, 0x45,
+ 0x46, 0x47, 0x48, 0x49, 0x4A, 0x53, 0x54, 0x55, 0x56, 0x57, 0x58, 0x59,
+ 0x5A, 0x63, 0x64, 0x65, 0x66, 0x67, 0x68, 0x69, 0x6A, 0x73, 0x74, 0x75,
+ 0x76, 0x77, 0x78, 0x79, 0x7A, 0x83, 0x84, 0x85, 0x86, 0x87, 0x88, 0x89,
+ 0x8A, 0x92, 0x93, 0x94, 0x95, 0x96, 0x97, 0x98, 0x99, 0x9A, 0xA2, 0xA3,
+ 0xA4, 0xA5, 0xA6, 0xA7, 0xA8, 0xA9, 0xAA, 0xB2, 0xB3, 0xB4, 0xB5, 0xB6,
+ 0xB7, 0xB8, 0xB9, 0xBA, 0xC2, 0xC3, 0xC4, 0xC5, 0xC6, 0xC7, 0xC8, 0xC9,
+ 0xCA, 0xD2, 0xD3, 0xD4, 0xD5, 0xD6, 0xD7, 0xD8, 0xD9, 0xDA, 0xE1, 0xE2,
+ 0xE3, 0xE4, 0xE5, 0xE6, 0xE7, 0xE8, 0xE9, 0xEA, 0xF1, 0xF2, 0xF3, 0xF4,
+ 0xF5, 0xF6, 0xF7, 0xF8, 0xF9, 0xFA, 0xFF, 0xDA, 0x00, 0x08, 0x01, 0x01,
+ 0x00, 0x00, 0x3F, 0x00, 0xFB, 0xD5, 0xDB, 0x20, 0xA8, 0xA0, 0x02, 0x80,
+ 0x0A, 0x00, 0xFF, 0xD9
+])
+
`@pytest.fixture`
def mock_image(tmp_path):
# Setup: Create a fake image file and a fake thumbnail
img_dir = tmp_path / "images"
img_dir.mkdir()
img_path = img_dir / "test_image.jpg"
- img_path.write_bytes(b"fake image data")
+ img_path.write_bytes(MINIMAL_JPEG)
thumb_dir = tmp_path / "thumbnails"
thumb_dir.mkdir()
thumb_path = thumb_dir / "test_thumb.jpg"
- thumb_path.write_bytes(b"fake thumb data")
+ thumb_path.write_bytes(MINIMAL_JPEG)Alternatively, use PIL to generate a valid image:
from PIL import Image as PILImage
import io
# In fixture:
img = PILImage.new('RGB', (1, 1), color='red')
buffer = io.BytesIO()
img.save(buffer, format='JPEG')
img_path.write_bytes(buffer.getvalue())🤖 Prompt for AI Agents
In `@backend/tests/test_image_deletion.py` around lines 44 - 51, The test writes
non-image bytes which causes PIL.Image.open() in image_util_generate_thumbnail
(called by image_util_prepare_image_records) to fail and masks the real
tombstone behavior; replace the raw b"fake image data" writes in the fixture
used by test_tombstone_prevents_indexing with a minimal valid JPEG/PNG byte
payload (create a 1x1 image via PIL.Image.new and save it into an in-memory
buffer, then write buffer.getvalue() to both img_path and thumb_path) and add
the necessary PIL import to the test so thumbnail generation succeeds and the
tombstone logic is actually exercised.
| "scope": [ | ||
| "**" | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Tauri assetProtocol scope security best practices ["**"]
💡 Result:
Here are concise best practices for Tauri's assetProtocol scope (including using "**" wildcards) with sources.
- Enable assetProtocol only if needed and keep scope minimal — follow principle of least privilege (avoid "**" unless absolutely required). [1][2]
- Prefer explicit paths or predefined scope variables (e.g., $CACHE, $HOME) instead of broad globs; more specific patterns reduce attack surface. [1][3]
- If you must use globs/wildcards, use deny entries to block sensitive paths (hidden files, Tauri runtime files) and validate merging of allow/deny. Don’t rely on allow-only "**". [1][3]
- Remember platform differences: hidden-dot folders (e.g., .cache) may be excluded unless explicitly listed (or requireLiteralLeadingDot configured) — include them explicitly if needed. Testing on each target OS is required. [2]
- Combine assetProtocol scope with a strict Content-Security-Policy (include media-src/img-src for asset:), capabilities, and command scopes — CSP must allow asset: if you serve images/media from asset://. [4][2]
- Audit and enforce scope validation in Rust command/plugin implementations; command authors must prevent scope bypasses and treat deny rules as higher priority. [1][5]
- Prefer runtime or plugin-based persisted/managed scopes (persisted-scope plugin) rather than enabling global "**" at release; persist/restore fine-grained scopes when appropriate. [6]
- Avoid exposing wildcards or remote domains for IPC or assets; wildcards or permissive domains can enable domain takeover or remote code access — use explicit domains and caution with dangerousRemoteUrlIpcAccess. [7]
Sources:
[1] Tauri — Command Scopes (security/scope) (official docs).
[2] Tauri GitHub discussion about asset protocol, hidden folders and scope patterns.
[3] Tauri — Configuration / Security / assetProtocol (official config reference).
[4] Community notes / examples about CSP needing media-src for asset: (forum/Reddit).
[5] Tauri docs note: scope validation must be audited (command/plugin guidance).
[6] Tauri persisted-scope plugin docs.
[7] Tauri v1 blog on risks of wildcards/remote domain exposure.
If you want, I can: (A) review your current tauri.conf.json and capability files and give specific fixes, or (B) produce a minimal secure example config that allows a single asset folder (including hidden files) for Linux + macOS + Windows.
🏁 Script executed:
# Explore the tauri.conf.json file and its context
cat -n frontend/src-tauri/tauri.conf.json | head -100Repository: AOSSIE-Org/PictoPy
Length of output: 2561
🏁 Script executed:
# Find project documentation to understand app purpose
find . -maxdepth 2 -type f \( -name "README*" -o -name "*.md" \) | head -20Repository: AOSSIE-Org/PictoPy
Length of output: 278
🏁 Script executed:
# Search for any documentation or comments about asset protocol scope
rg -i "assetProtocol|asset.*scope" -B 3 -A 3 --type json --type md --type txtRepository: AOSSIE-Org/PictoPy
Length of output: 409
Restrict assetProtocol scope to necessary directories.
The assetProtocol.scope set to ["**"] grants unrestricted access to all file paths, violating Tauri's principle of least privilege. For a photo management app, this should be restricted to:
- Standard photo directories using predefined variables (
$PICTURES,$DOWNLOADS,$DOCUMENTS), or - User-selected folders obtained through file picker dialogs, or
- Use Tauri's persisted-scope plugin for runtime-managed access.
If a broad scope is necessary, add explicit deny entries to block sensitive system files (hidden config directories, system paths, etc.) and audit scope validation in backend implementations. Reference Tauri's command scopes and assetProtocol security documentation for secure configuration patterns.
🤖 Prompt for AI Agents
In `@frontend/src-tauri/tauri.conf.json` around lines 69 - 71, The
assetProtocol.scope value currently set to ["**"] is overly permissive; update
the tauri.conf.json assetProtocol.scope to restrict paths to only required
directories (e.g., use predefined variables like $PICTURES, $DOWNLOADS,
$DOCUMENTS or explicit user-selected folders) or switch to Tauri's
persisted-scope plugin to grant runtime-managed access; if broad patterns are
absolutely required, add explicit deny entries for sensitive system and hidden
config paths and ensure any backend code that validates scope uses the tightened
scope values instead of assuming global access.
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
✏️ Tip: You can customize this high-level summary in your review settings.