Right-Click Context Menu Actions for Images (Favorite, Copy, View Details) #900

coderabbitai · 2026-01-14T19:34:25Z

⚠️ Potential issue | 🟡 Minor

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.

coderabbitai · 2026-01-14T19:34:25Z

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Find the implementation of db_add_to_deleted_images rg -n "def db_add_to_deleted_images" --type=py -A 10

Repository: 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 2

Repository: 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 1

Repository: 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.py

Repository: 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 checked

If 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.

-Original file line number
+Diff line change
@@ -1,6 +1,6 @@
     # Standard library imports
     import sqlite3
-    from typing import Any, List, Mapping, Tuple, TypedDict, Union
+    from typing import Any, List, Mapping, Optional, Tuple, TypedDict, Union
     # App-specific imports
     from app.config.settings import (
@@ Expand Down Expand Up / @@ -82,6 +82,16 @@ def db_create_images_table() -> None: @@
         """
         )
+        # Create deleted_images table (tombstone)
+        cursor.execute(
+            """
+            CREATE TABLE IF NOT EXISTS deleted_images (
+                path TEXT PRIMARY KEY,
+                deleted_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP
+            )
+            """
+        )
         conn.commit()
         conn.close()
@@ Expand Down Expand Up @@
             return False
         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),
+                "thumbnailPath": thumbnail_path,
+                "metadata": image_util_parse_metadata(metadata),
+                "isTagged": bool(is_tagged),
+                "isFavourite": bool(is_favourite),
+            }
+        finally:
+            conn.close()
+    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()
+    def db_is_image_deleted(path: str) -> bool:
+        """
+        Check if a path exists in the deleted_images tombstone table.
+        """
+        conn = _connect()
+        cursor = conn.cursor()
+        try:
+            cursor.execute("SELECT 1 FROM deleted_images WHERE path = ?", (path,))
+            return cursor.fetchone() is not None
+        finally:
+            conn.close()

-Original file line number
+Diff line change
@@ Expand Up / @@ -243,16 +243,9 @@ def emit(self, record: logging.LogRecord) -> None: @@
             # Create a message that includes the original module in the format
             msg = record.getMessage()
-            record.msg = f"[{module_name}] {msg}"
-            record.args = ()
-            # Clear exception / stack info to avoid duplicate traces
-            record.exc_info = None
-            record.stack_info = None
-            root_logger = logging.getLogger()
-            for handler in root_logger.handlers:
-                if handler is not self:
-                    handler.handle(record)
+            # 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}")
     def configure_uvicorn_logging(component_name: str) -> None:
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -1,10 +1,15 @@
     from fastapi import APIRouter, HTTPException, Query, status
     from typing import List, Optional
-    from app.database.images import db_get_all_images
-    from app.schemas.images import ErrorResponse
-    from app.utils.images import image_util_parse_metadata
+    from app.database.images import (
+        db_get_all_images,
+        db_toggle_image_favourite_status,
+        db_get_image_by_id,
+        db_delete_images_by_ids,
+        db_add_to_deleted_images,
+    )
+    from app.schemas.images import ErrorResponse, DeleteImageResponse
+    from app.utils.images import image_util_parse_metadata, image_util_delete_image_files
     from pydantic import BaseModel
-    from app.database.images import db_toggle_image_favourite_status
     from app.logging.setup_logging import get_logger
     # Initialize logger
@@ Expand Down Expand Up / @@ -128,3 +133,62 @@ class ImageInfoResponse(BaseModel): @@
         isTagged: bool
         isFavourite: bool
         tags: Optional[List[str]] = None
+    @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:
+. Fetch image record from DB.
+. Add to 'deleted_images' tombstone (prevents re-sync).
+. Delete physical files (original + thumbnail).
+. 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(),
+            )

-Original file line number
+Diff line change
@@ Expand Up / @@ -15,6 +15,7 @@ @@
         db_insert_image_classes_batch,
         db_get_images_by_folder_ids,
         db_delete_images_by_ids,
+        db_is_image_deleted,
     )
     from app.models.FaceDetector import FaceDetector
     from app.models.ObjectClassifier import ObjectClassifier
@@ Expand Down Expand Up / @@ -151,6 +152,11 @@ def image_util_prepare_image_records( @@
         """
         image_records = []
         for image_path in image_files:
+            # Check if image was explicitly deleted before (tombstone)
+            if db_is_image_deleted(image_path):
+                logger.debug(f"Skipping tombstoned image: {image_path}")
+                continue
             folder_id = image_util_find_folder_id_for_image(image_path, folder_path_to_id)
             if not folder_id:
@@ Expand Down Expand Up @@
             return db_metadata
         return {}
+    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

-Original file line number
+Diff line change
@@ -0,0 +1,121 @@
+    import os
+    import pytest
+    from fastapi.testclient import TestClient
+    import sys
+    from pathlib import Path
+    # Add backend to path to allow imports and find main.py
+    sys.path.append(str(Path(__file__).parent.parent))
+    from main import app
+    from app.database.images import db_get_image_by_id, db_is_image_deleted, db_bulk_insert_images, db_create_images_table
+    from app.config.settings import DATABASE_PATH
+    import sqlite3
+    import uuid
+    import json
+    client = TestClient(app)
+    @pytest.fixture(autouse=True)
+    def setup_db():
+        # Make sure images table (and deleted_images) is created
+        db_create_images_table()
+        # Use config-defined DATABASE_PATH
+        conn = sqlite3.connect(DATABASE_PATH)
+        cursor = conn.cursor()
+        # Ensure folders table exists (might already be created by conftest)
+        cursor.execute("""
+            CREATE TABLE IF NOT EXISTS folders (
+                folder_id TEXT 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')")
+        conn.commit()
+        conn.close()
+    @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")
+        thumb_dir = tmp_path / "thumbnails"
+        thumb_dir.mkdir()
+        thumb_path = thumb_dir / "test_thumb.jpg"
+        thumb_path.write_bytes(b"fake thumb data")
+        image_id = str(uuid.uuid4())
+        image_record = {
+            "id": image_id,
+            "path": str(img_path),
+            "folder_id": 1,
+            "thumbnailPath": str(thumb_path),
+            "metadata": json.dumps({
+                "name": "test_image.jpg",
+                "width": 100,
+                "height": 100,
+                "file_size": 100,
+                "file_location": str(img_path),
+                "item_type": "image/jpeg"
+            }),
+            "isTagged": False
+        }
+        # Insert into DB
+        db_bulk_insert_images([image_record])
+        return image_record
+    def test_delete_image_success(mock_image):
+        image_id = mock_image["id"]
+        img_path = mock_image["path"]
+        thumb_path = mock_image["thumbnailPath"]
+        # 1. Verify existence before deletion
+        assert os.path.exists(img_path)
+        assert os.path.exists(thumb_path)
+        assert db_get_image_by_id(image_id) is not None
+        # 2. Call DELETE API
+        response = client.delete(f"/images/{image_id}")
+        assert response.status_code == 200
+        data = response.json()
+        assert data["success"] is True
+        # 3. Verify deletion
+        assert not os.path.exists(img_path)
+        assert not os.path.exists(thumb_path)
+        assert db_get_image_by_id(image_id) is None
+        assert db_is_image_deleted(img_path) is True
+    def test_delete_image_not_found():
+        response = client.delete("/images/non-existent-id")
+        assert response.status_code == 404
+    def test_tombstone_prevents_indexing(mock_image):
+        image_id = mock_image["id"]
+        img_path = mock_image["path"]
+        # Delete the image first
+        client.delete(f"/images/{image_id}")
+        assert db_is_image_deleted(img_path) is True
+        # Re-create the file manually
+        os.makedirs(os.path.dirname(img_path), exist_ok=True)
+        with open(img_path, "wb") as f:
+            f.write(b"recreated file")
+        # Try to re-index it via utilities
+        from app.utils.images import image_util_prepare_image_records
+        # Mocking folder_path_to_id
+        folder_path = str(Path(img_path).parent)
+        records = image_util_prepare_image_records([str(img_path)], {folder_path: 1})
+        # It should skip it because of the tombstone
+        assert len(records) == 0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Right-Click Context Menu Actions for Images (Favorite, Copy, View Details) #900

Uh oh!

Diff view

Diff view

There are no files selected for viewing

coderabbitai bot Jan 14, 2026

Uh oh!

coderabbitai bot Jan 14, 2026

Uh oh!

Uh oh!

Right-Click Context Menu Actions for Images *(Favorite, Copy, View Details)* #900

Uh oh!

Right-Click Context Menu Actions for Images *(Favorite, Copy, View Details)* #900

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

coderabbitai bot Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Right-Click Context Menu Actions for Images (Favorite, Copy, View Details) #900

Right-Click Context Menu Actions for Images (Favorite, Copy, View Details) #900