From 624c65909eaf074263e600de69f4719366b39a4c Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Thu, 16 Jul 2026 16:24:04 -0400 Subject: [PATCH] fix: surface actionable delete-failure messages instead of a bare HTTP 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting an image from a read-only folder popped a dialog reading "Failed to delete: HTTP 500 Internal Server Error for delete_image/..." — the server knew it was a permissions problem but the reason never reached the user. Backend: the file-removal step of delete_image now translates OS failures into HTTP errors with actionable detail — PermissionError becomes a 403 naming the file and pointing at write permissions; send2trash's TrashPermissionError becomes a 403 suggesting the 'Just Delete' setting (the file may be deletable even when the trash folder isn't usable); other OSErrors keep the 500 but carry the OS reason and the same Settings hint when trashing was the failure. Frontend: new errorDetail() helper in utils.js prefers the server's {detail: ...} explanation over HttpError's generic message; the delete alerts in control-panel.js and bookmarks.js use it. Co-Authored-By: Claude Fable 5 --- photomap/backend/routers/index.py | 52 ++++++++++- .../frontend/static/javascript/bookmarks.js | 4 +- .../static/javascript/control-panel.js | 4 +- photomap/frontend/static/javascript/utils.js | 13 +++ tests/backend/test_index.py | 86 +++++++++++++++++++ tests/frontend/utils.test.js | 22 +++++ 6 files changed, 173 insertions(+), 8 deletions(-) diff --git a/photomap/backend/routers/index.py b/photomap/backend/routers/index.py index 923bb38e..0560a27e 100644 --- a/photomap/backend/routers/index.py +++ b/photomap/backend/routers/index.py @@ -13,6 +13,7 @@ from fastapi.responses import JSONResponse from pydantic import BaseModel from send2trash import send2trash +from send2trash.exceptions import TrashPermissionError from .. import invokeai_client from ..config import get_config_manager @@ -278,6 +279,52 @@ async def index_metadata(album_config: AlbumDep) -> EmbeddingsIndexMetadata: ) +def _remove_image_file(image_path: Path, move_to_trash: bool) -> None: + """Trash or unlink ``image_path``, translating OS failures into HTTP + errors whose ``detail`` tells the user what to fix. + + Without this, a read-only folder surfaced as a bare + "HTTP 500 Internal Server Error" dialog with no hint that it was a + permissions problem. + """ + try: + if move_to_trash: + send2trash(str(image_path)) + else: + image_path.unlink() + except TrashPermissionError as e: + raise HTTPException( + status_code=403, + detail=( + f"Cannot move '{image_path.name}' to the trash: permission denied " + "creating or writing the trash folder. Fix its permissions, or " + 'switch "When deleting" to "Just Delete" in Settings to delete ' + "the file permanently." + ), + ) from e + except PermissionError as e: + raise HTTPException( + status_code=403, + detail=( + f"Cannot delete '{image_path.name}': permission denied. " + "The PhotoMap server needs write permission on the image file " + "and its containing folder." + ), + ) from e + except OSError as e: + action = "move to the trash" if move_to_trash else "delete" + reason = e.strerror or str(e) + hint = ( + ' Switching "When deleting" to "Just Delete" in Settings may help.' + if move_to_trash + else "" + ) + raise HTTPException( + status_code=500, + detail=f"Could not {action} '{image_path.name}': {reason}.{hint}", + ) from e + + @index_router.delete( "/delete_image/{album_key}/{index}", tags=["Index"], @@ -321,10 +368,7 @@ async def delete_image( raise HTTPException(status_code=404, detail="File not found") print(f"{'Trashing' if move_to_trash else 'Deleting'} image: {image_path}") - if move_to_trash: - send2trash(str(image_path)) - else: - image_path.unlink() + _remove_image_file(image_path, move_to_trash) # Remove from embeddings embeddings.remove_image_from_embeddings(index) diff --git a/photomap/frontend/static/javascript/bookmarks.js b/photomap/frontend/static/javascript/bookmarks.js index db4d2b43..7c616ff8 100644 --- a/photomap/frontend/static/javascript/bookmarks.js +++ b/photomap/frontend/static/javascript/bookmarks.js @@ -8,7 +8,7 @@ import { showConfirmModal } from "./modal-utils.js"; import { setSearchResults } from "./search.js"; import { slideState } from "./slide-state.js"; import { state } from "./state.js"; -import { fetchJson, hideSpinner, setCheckmarkOnIcon, showSpinner } from "./utils.js"; +import { errorDetail, fetchJson, hideSpinner, setCheckmarkOnIcon, showSpinner } from "./utils.js"; // SVG icons for bookmark actions const BOOKMARK_SVG = ` @@ -607,7 +607,7 @@ class BookmarkManager { this.clearBookmarks(); } catch (error) { console.error("Delete failed:", error); - alert(`Delete failed: ${error.message}`); + alert(`Delete failed: ${errorDetail(error)}`); } finally { hideSpinner(); } diff --git a/photomap/frontend/static/javascript/control-panel.js b/photomap/frontend/static/javascript/control-panel.js index f8e033e8..e42a7cfa 100644 --- a/photomap/frontend/static/javascript/control-panel.js +++ b/photomap/frontend/static/javascript/control-panel.js @@ -3,7 +3,7 @@ import { deleteImage, getIndexMetadata } from "./index.js"; import { getCurrentFilepath, getCurrentSlideIndex, slideState } from "./slide-state.js"; import { saveSettingsToLocalStorage, state } from "./state.js"; -import { hideSpinner, showSpinner } from "./utils.js"; +import { errorDetail, hideSpinner, showSpinner } from "./utils.js"; // Cache DOM elements let elements = {}; @@ -112,7 +112,7 @@ async function handleDeleteCurrentFile() { hideSpinner(); } catch (error) { hideSpinner(); - alert(`Failed to delete: ${error.message}`); + alert(`Failed to delete: ${errorDetail(error)}`); console.error("Delete failed:", error); } } diff --git a/photomap/frontend/static/javascript/utils.js b/photomap/frontend/static/javascript/utils.js index 78a672bd..c439d391 100644 --- a/photomap/frontend/static/javascript/utils.js +++ b/photomap/frontend/static/javascript/utils.js @@ -173,6 +173,19 @@ export class HttpError extends Error { } } +/** + * The message to show a user for a failed request: the server's explanation + * (FastAPI's ``{"detail": "..."}``) when there is one, else the error's own + * message. Alerts that show ``error.message`` directly print the unhelpful + * "HTTP 500 Internal Server Error for " and bury the actual reason. + */ +export function errorDetail(error) { + if (error instanceof HttpError && typeof error.body?.detail === "string") { + return error.body.detail; + } + return error.message; +} + /** * Thin wrapper around ``fetch`` + ``response.json()`` that throws * :class:`HttpError` on non-2xx and returns the parsed body on 2xx. diff --git a/tests/backend/test_index.py b/tests/backend/test_index.py index 17a083f1..5e76725b 100644 --- a/tests/backend/test_index.py +++ b/tests/backend/test_index.py @@ -130,6 +130,92 @@ def test_delete_image( ).exists(), "Image file should be deleted" +def test_delete_image_permission_error_names_file_and_cause( + client: TestClient, new_album: dict, monkeypatch: pytest.MonkeyPatch +): + """A file the server can't remove must surface as a 403 whose detail names + the file and says it's a permissions problem — not the generic + "HTTP 500 Internal Server Error" the alert dialog used to show.""" + build_index(client, new_album) + album_key = new_album["key"] + filename = client.get(f"/retrieve_image/{album_key}/0").json()["filename"] + + def deny_unlink(self, missing_ok=False): + raise PermissionError(13, "Permission denied", str(self)) + + monkeypatch.setattr(Path, "unlink", deny_unlink) + response = client.delete( + f"/delete_image/{album_key}/0", params={"move_to_trash": False} + ) + monkeypatch.undo() + + assert response.status_code == 403 + detail = response.json()["detail"] + assert filename in detail + assert "permission denied" in detail.lower() + assert "write permission" in detail + + # The failed delete must leave both the file and its index row in place. + assert (Path(new_album["image_paths"][0]) / filename).exists() + metadata = client.get(f"/index_metadata/{album_key}").json() + assert metadata["filename_count"] == TEST_IMAGE_COUNT + + +def test_delete_image_trash_permission_error_points_at_settings( + client: TestClient, new_album: dict, monkeypatch: pytest.MonkeyPatch +): + """send2trash's TrashPermissionError means the file itself may be + deletable but the trash folder isn't usable — the message must offer the + "Just Delete" setting as the way out.""" + from send2trash.exceptions import TrashPermissionError + + from photomap.backend.routers import index as index_router_module + + build_index(client, new_album) + album_key = new_album["key"] + filename = client.get(f"/retrieve_image/{album_key}/0").json()["filename"] + + def broken_trash(path): + raise TrashPermissionError(path) + + monkeypatch.setattr(index_router_module, "send2trash", broken_trash) + + response = client.delete(f"/delete_image/{album_key}/0") # move_to_trash default + assert response.status_code == 403 + detail = response.json()["detail"] + assert filename in detail + assert "trash" in detail.lower() + assert "Just Delete" in detail + assert (Path(new_album["image_paths"][0]) / filename).exists() + + +def test_delete_image_trash_oserror_surfaces_reason( + client: TestClient, new_album: dict, monkeypatch: pytest.MonkeyPatch +): + """Non-permission trash failures (e.g. no trash dir on the file's mount) + keep the 500 but the detail must carry the OS reason and the Settings + hint instead of a bare 'Failed to delete file'.""" + import errno + + from photomap.backend.routers import index as index_router_module + + build_index(client, new_album) + album_key = new_album["key"] + filename = client.get(f"/retrieve_image/{album_key}/0").json()["filename"] + + def broken_trash(path): + raise OSError(errno.EXDEV, "Invalid cross-device link") + + monkeypatch.setattr(index_router_module, "send2trash", broken_trash) + + response = client.delete(f"/delete_image/{album_key}/0") + assert response.status_code == 500 + detail = response.json()["detail"] + assert filename in detail + assert "Invalid cross-device link" in detail + assert "Just Delete" in detail + + def test_npz_rewrites_preserve_non_per_image_keys(tmp_path: Path): """``remove_image_from_embeddings`` and ``update_image_path`` rewrite the whole ``.npz``; they must carry over ``model_id``, ``embedding_dim``, and diff --git a/tests/frontend/utils.test.js b/tests/frontend/utils.test.js index d56d5344..8c0f181f 100644 --- a/tests/frontend/utils.test.js +++ b/tests/frontend/utils.test.js @@ -9,6 +9,8 @@ import { debounce, getPercentile, setCheckmarkOnIcon, + errorDetail, + HttpError, } from "../../photomap/frontend/static/javascript/utils.js"; describe("utils.js", () => { @@ -295,4 +297,24 @@ describe("utils.js", () => { expect(() => setCheckmarkOnIcon(undefined, false)).not.toThrow(); }); }); + + describe("errorDetail", () => { + it("prefers the server's detail string for an HttpError", () => { + const err = new HttpError(403, "Forbidden", "delete_image/family/4425", { + detail: "Cannot delete 'img.jpg': permission denied.", + }); + expect(errorDetail(err)).toBe("Cannot delete 'img.jpg': permission denied."); + }); + + it("falls back to the error message when there is no string detail", () => { + // No body at all (e.g. reading the response body failed). + expect(errorDetail(new HttpError(500, "Internal Server Error", "u", undefined))).toBe( + "HTTP 500 Internal Server Error for u" + ); + // FastAPI validation errors put a list in detail — not showable as-is. + expect(errorDetail(new HttpError(422, "", "u", { detail: [{ loc: [] }] }))).toBe("HTTP 422 for u"); + // Non-HTTP errors pass straight through. + expect(errorDetail(new Error("plain failure"))).toBe("plain failure"); + }); + }); });