From b994170383cb2548a3d46edaacb03b68d56f4b48 Mon Sep 17 00:00:00 2001 From: FloeHetling Date: Wed, 7 Oct 2026 01:16:55 +0300 Subject: [PATCH 1/2] feat(gallery): optionally protect starred media from deletion Add a "Protect starred media" setting (off by default). While it is on, deleting images or videos skips starred ones, and deleting a board with its media moves starred items to Uncategorized instead. Add `delete_starred` to the image, video and board delete endpoints and report skipped items in `starred_skipped`; clients that omit it keep the old behavior. --- .../06.Gallery/gallery-reference.mdx | 2 + .../Users Guide/06.Gallery/introduction.mdx | 2 + invokeai/app/api/routers/boards.py | 27 ++- invokeai/app/api/routers/images.py | 32 +++- invokeai/app/api/routers/videos.py | 35 +++- invokeai/app/services/images/images_base.py | 19 +- invokeai/app/services/images/images_common.py | 4 + .../app/services/images/images_default.py | 22 ++- .../app/services/shared/bulk_media_delete.py | 28 ++- invokeai/app/services/videos/videos_base.py | 19 +- invokeai/app/services/videos/videos_common.py | 4 + .../app/services/videos/videos_default.py | 22 ++- invokeai/frontend/api/openapi.json | 171 ++++++++++++++++-- invokeai/frontend/api/schema.ts | 82 ++++++++- .../frontend/webv2/public/locales/en.json | 12 ++ .../webv2/src/app/GalleryUiAdapter.tsx | 4 + .../webv2/src/features/gallery/core/items.ts | 3 + .../webv2/src/features/gallery/core/types.ts | 9 + .../src/features/gallery/data/backend.test.ts | 68 +++++++ .../src/features/gallery/data/backend.ts | 52 +++++- .../gallery/data/organization.test.ts | 59 ++++++ .../src/features/gallery/data/queries.ts | 24 +++ .../webv2/src/features/gallery/index.ts | 29 ++- .../ui/GalleryBoardMenu.browser.test.tsx | 101 ++++++++--- .../features/gallery/ui/GalleryBoardMenu.tsx | 21 +++ .../ui/GalleryItemSearch.browser.test.tsx | 1 + .../ui/GalleryLoadFailures.browser.test.tsx | 1 + .../features/gallery/ui/GalleryUiContext.tsx | 2 + .../ui/GalleryWidgetView.browser.test.tsx | 1 + .../ui/useGalleryActions.browser.test.tsx | 59 ++++++ .../features/gallery/ui/useGalleryActions.ts | 40 +++- .../useGalleryUploadAction.browser.test.tsx | 1 + .../webv2/src/workbench/WorkbenchRuntime.tsx | 2 + .../useImageActions.browser.test.tsx | 158 +++++++++++++++- .../image-actions/useImageActions.ts | 94 +++++++++- ...nchCommandPalette.pending.browser.test.tsx | 2 +- .../workbench/projects/useProjectActions.ts | 2 + .../settings/applicationContributions.ts | 1 + .../webv2/src/workbench/settings/contracts.ts | 2 + .../src/workbench/settings/store.test.ts | 8 + .../webv2/src/workbench/settings/store.ts | 5 + .../src/workbench/workbenchState.test.ts | 6 + .../webv2/src/workbench/workbenchState.ts | 2 + tests/app/routers/test_boards_multiuser.py | 71 ++++++-- tests/app/routers/test_images.py | 135 +++++++++++++- .../routers/test_multiuser_authorization.py | 2 +- tests/app/routers/test_videos_multiuser.py | 91 ++++++++++ .../services/images/test_images_default.py | 112 +++++++++++- .../test_intermediates_service.py | 2 +- .../services/shared/test_bulk_media_delete.py | 93 ++++++++++ .../services/videos/test_videos_default.py | 69 ++++++- 51 files changed, 1659 insertions(+), 154 deletions(-) create mode 100644 tests/app/services/shared/test_bulk_media_delete.py diff --git a/docs/src/content/docs/Users Guide/06.Gallery/gallery-reference.mdx b/docs/src/content/docs/Users Guide/06.Gallery/gallery-reference.mdx index 8d954d225e8..811f1941964 100644 --- a/docs/src/content/docs/Users Guide/06.Gallery/gallery-reference.mdx +++ b/docs/src/content/docs/Users Guide/06.Gallery/gallery-reference.mdx @@ -54,6 +54,8 @@ Choose **Delete Board** from the board's menu. The confirmation dialog shows how **Delete Board and Media** cannot be undone. ::: +With **Protect starred media** turned on in Settings, **Delete Board and Media** keeps the board's starred images and videos: they move to **Uncategorized** while the board and everything else on it is deleted. The dialog says how many starred items will be kept. See [Deleting media](/users-guide/gallery/introduction/#deleting-media). + Queued or running jobs that were sending results to the deleted board send them to **Uncategorized** instead. A project's Inbox cannot be deleted from the Gallery; delete the project instead, which also asks what to do with the project's other boards. On a multi-user installation, only a board's owner or an administrator can delete it. When an owner deletes a shared board with its media, only their own media is deleted and other people's items move to Uncategorized; an administrator's deletion removes everything. If some files cannot be removed, those items are kept in Uncategorized and a warning says how many. diff --git a/docs/src/content/docs/Users Guide/06.Gallery/introduction.mdx b/docs/src/content/docs/Users Guide/06.Gallery/introduction.mdx index cf5b666bb01..2bb90edef56 100644 --- a/docs/src/content/docs/Users Guide/06.Gallery/introduction.mdx +++ b/docs/src/content/docs/Users Guide/06.Gallery/introduction.mdx @@ -150,6 +150,8 @@ Drag items onto a board in the board list; if the list is hidden, it opens while Press **Delete** (or **Backspace**), choose **Delete Image**, **Delete Video** or **Delete Selection** from the menu, or use the delete button in the selection bar. Deleting permanently removes the items from every board and from disk, and cannot be undone. A confirmation asks first. Tick **Don't ask me again** and confirm to stop asking for ordinary deletions (cancelling leaves it on); you can turn it back on, or off, with **Confirm image deletion** in Settings under Gallery. +To keep starred items safe from accidental deletion, turn on **Protect starred media** in the **Gallery** section of Settings. While it is on, deleting skips starred images and videos: from a mixed selection only the unstarred items are deleted, and when everything selected is starred nothing is deleted. If **Confirm image deletion** is on, a notice says how many starred items were kept; otherwise they are kept silently. Unstar an item to delete it. The setting is off by default. + When the open project still uses something you are deleting, for example as a canvas layer, a reference image, an upscale or video input, or in a workflow, the confirmation always appears and lists where, even if you turned confirmations off. Only the open project's current work is checked: other projects, undo history and canvas snapshots are not, so check before you delete media you still need there. If deleting is interrupted, InvokeAI cleans up any leftover files the next time it starts. ## Related pages diff --git a/invokeai/app/api/routers/boards.py b/invokeai/app/api/routers/boards.py index 24a9b130e0b..9c1522145be 100644 --- a/invokeai/app/api/routers/boards.py +++ b/invokeai/app/api/routers/boards.py @@ -45,6 +45,14 @@ class DeleteBoardResult(BaseModel): default_factory=list, description="The names of videos that could not be deleted and became uncategorized.", ) + starred_images_skipped: list[str] = Field( + default_factory=list, + description="The names of starred images that were protected and became uncategorized.", + ) + starred_videos_skipped: list[str] = Field( + default_factory=list, + description="The names of starred videos that were protected and became uncategorized.", + ) @boards_router.post( @@ -145,6 +153,7 @@ def delete_board( include_images: Optional[bool] = Query( description="Permanently delete all images and videos on the board", default=False ), + delete_starred: bool = Query(default=True, description="Whether to allow deletion of starred media"), ) -> DeleteBoardResult: """Deletes a board (user must have access to it)""" try: @@ -206,11 +215,19 @@ def delete_board( # as failures. This is the ground truth — reconstructing failures by diffing a # router-side board listing against the deleted names would double the DB work and # misreport items moved or deleted concurrently between the two queries. - deleted_images, failed_images = ApiDependencies.invoker.services.images.delete_images_by_names( - board_image_names + ( + deleted_images, + failed_images, + starred_images_skipped, + ) = ApiDependencies.invoker.services.images.delete_images_by_names( + board_image_names, delete_starred=delete_starred ) - deleted_videos, failed_videos = ApiDependencies.invoker.services.videos.delete_videos_by_names( - board_video_names + ( + deleted_videos, + failed_videos, + starred_videos_skipped, + ) = ApiDependencies.invoker.services.videos.delete_videos_by_names( + board_video_names, delete_starred=delete_starred ) return DeleteBoardResult( board_id=board_id, @@ -220,6 +237,8 @@ def delete_board( deleted_videos=deleted_videos, failed_images=failed_images, failed_videos=failed_videos, + starred_images_skipped=starred_images_skipped, + starred_videos_skipped=starred_videos_skipped, ) return DeleteBoardResult( diff --git a/invokeai/app/api/routers/images.py b/invokeai/app/api/routers/images.py index 874efd5f424..8beb6c21eb7 100644 --- a/invokeai/app/api/routers/images.py +++ b/invokeai/app/api/routers/images.py @@ -239,6 +239,7 @@ def create_image_upload_entry( def delete_image( current_user: CurrentUserOrDefault, image_name: str = Path(description="The name of the image to delete"), + delete_starred: bool = Query(default=True, description="Whether to allow deletion of starred images"), ) -> DeleteImagesResult: """Deletes an image""" _assert_image_owner(image_name, current_user) @@ -259,7 +260,7 @@ def delete_image( board_id = image_dto.board_id or "none" try: - ApiDependencies.invoker.services.images.delete(image_name) + was_deleted = ApiDependencies.invoker.services.images.delete(image_name, delete_starred=delete_starred) except ImageRecordNotFoundException: # Another request deleted the image between the lookup above and the service call. The # image is gone, which is what the client asked for — answer as the lookup would have. @@ -268,11 +269,12 @@ def delete_image( raise HTTPException(status_code=500, detail="Failed to delete image") return DeleteImagesResult( - deleted_images=[image_name], + deleted_images=[image_name] if was_deleted else [], # Every failure path above raises, so a returned result always describes a completed - # delete; nothing can land in ``failed_images``. + # delete or a protected starred image; nothing can land in ``failed_images``. failed_images=[], - affected_boards=[board_id], + affected_boards=[board_id] if was_deleted else [], + starred_skipped=[] if was_deleted else [image_name], ) @@ -571,6 +573,7 @@ def delete_images_from_list( image_names: list[ImageName] = Body( description="The list of names of images to delete", embed=True, max_length=MAX_IMAGE_BATCH_SIZE ), + delete_starred: bool = Body(default=True, description="Whether to allow deletion of starred images"), ) -> DeleteImagesResult: try: assert_image_move_maintenance_inactive() @@ -587,6 +590,7 @@ def delete_images_from_list( deleted_images: set[str] = set() failed_images: set[str] = set() affected_boards: set[str] = set() + starred_skipped: set[str] = set() # Dedup while preserving order: a name repeated in the request would otherwise # be processed twice, and the second pass's not-found error would land the same # name in both deleted_images and failed_images. @@ -602,9 +606,11 @@ def delete_images_from_list( _assert_image_owner(image_name, current_user) image_dto = ApiDependencies.invoker.services.images.get_dto(image_name) board_id = image_dto.board_id or "none" - ApiDependencies.invoker.services.images.delete(image_name) - deleted_images.add(image_name) - affected_boards.add(board_id) + if ApiDependencies.invoker.services.images.delete(image_name, delete_starred=delete_starred): + deleted_images.add(image_name) + affected_boards.add(board_id) + else: + starred_skipped.add(image_name) except HTTPException: continue except ImageRecordNotFoundException: @@ -642,6 +648,7 @@ def delete_images_from_list( deleted_images=list(deleted_images), failed_images=list(failed_images), affected_boards=list(affected_boards), + starred_skipped=list(starred_skipped), ) except HTTPException: raise @@ -652,6 +659,7 @@ def delete_images_from_list( @images_router.delete("/uncategorized", operation_id="delete_uncategorized_images", response_model=DeleteImagesResult) def delete_uncategorized_images( current_user: CurrentUserOrDefault, + delete_starred: bool = Query(default=True, description="Whether to allow deletion of starred images"), ) -> DeleteImagesResult: """Deletes all uncategorized images owned by the current user (or all if admin)""" assert_image_move_maintenance_inactive() @@ -664,12 +672,15 @@ def delete_uncategorized_images( deleted_images: set[str] = set() failed_images: set[str] = set() affected_boards: set[str] = set() + starred_skipped: set[str] = set() for image_name in image_names: try: _assert_image_owner(image_name, current_user) - ApiDependencies.invoker.services.images.delete(image_name) - deleted_images.add(image_name) - affected_boards.add("none") + if ApiDependencies.invoker.services.images.delete(image_name, delete_starred=delete_starred): + deleted_images.add(image_name) + affected_boards.add("none") + else: + starred_skipped.add(image_name) except HTTPException: # Skip images not owned by the current user pass @@ -679,6 +690,7 @@ def delete_uncategorized_images( deleted_images=list(deleted_images), failed_images=list(failed_images), affected_boards=list(affected_boards), + starred_skipped=list(starred_skipped), ) except Exception: raise HTTPException(status_code=500, detail="Failed to delete images") diff --git a/invokeai/app/api/routers/videos.py b/invokeai/app/api/routers/videos.py index 70bfa3456bc..5018f132da6 100644 --- a/invokeai/app/api/routers/videos.py +++ b/invokeai/app/api/routers/videos.py @@ -143,6 +143,10 @@ class VideoNamesBatch(BaseModel): ) +class DeleteVideosBatch(VideoNamesBatch): + delete_starred: bool = Field(default=True, description="Whether to allow deletion of starred videos") + + def _get_video_cache_control() -> str: if ApiDependencies.invoker.services.configuration.multiuser: return "private, no-store" @@ -620,6 +624,7 @@ async def upload_video( def delete_video( current_user: CurrentUserOrDefault, video_name: str = PathParam(description="The name of the video to delete"), + delete_starred: bool = Query(default=True, description="Whether to allow deletion of starred videos"), ) -> DeleteVideosResult: _assert_video_owner(video_name, current_user) @@ -635,21 +640,22 @@ def delete_video( board_id = video_dto.board_id or "none" try: - ApiDependencies.invoker.services.videos.delete(video_name) + was_deleted = ApiDependencies.invoker.services.videos.delete(video_name, delete_starred=delete_starred) except Exception: raise HTTPException(status_code=500, detail="Failed to delete video") return DeleteVideosResult( - deleted_videos=[video_name], + deleted_videos=[video_name] if was_deleted else [], failed_videos=[], - affected_boards=[board_id], + affected_boards=[board_id] if was_deleted else [], + starred_skipped=[] if was_deleted else [video_name], ) @videos_router.post("/delete", operation_id="delete_videos_from_list", response_model=DeleteVideosResult) def delete_videos_from_list( current_user: CurrentUserOrDefault, - batch: VideoNamesBatch, + batch: DeleteVideosBatch, ) -> DeleteVideosResult: # Skip — but do not re-raise — auth failures so a foreign name mid-batch doesn't # discard the response payload for items the caller had already legitimately deleted. @@ -663,6 +669,7 @@ def delete_videos_from_list( deleted_videos: set[str] = set() failed_videos: set[str] = set() affected_boards: set[str] = set() + starred_skipped: set[str] = set() # Dedup while preserving order: a name repeated in the request would otherwise be # processed twice, and the second pass's not-found error would land the same name # in both deleted_videos and failed_videos. @@ -671,9 +678,11 @@ def delete_videos_from_list( _assert_video_owner(video_name, current_user) video_dto = ApiDependencies.invoker.services.videos.get_dto(video_name) board_id = video_dto.board_id or "none" - ApiDependencies.invoker.services.videos.delete(video_name) - deleted_videos.add(video_name) - affected_boards.add(board_id) + if ApiDependencies.invoker.services.videos.delete(video_name, delete_starred=batch.delete_starred): + deleted_videos.add(video_name) + affected_boards.add(board_id) + else: + starred_skipped.add(video_name) except HTTPException: continue except Exception: @@ -682,12 +691,14 @@ def delete_videos_from_list( deleted_videos=list(deleted_videos), failed_videos=list(failed_videos), affected_boards=list(affected_boards), + starred_skipped=list(starred_skipped), ) @videos_router.delete("/uncategorized", operation_id="delete_uncategorized_videos", response_model=DeleteVideosResult) def delete_uncategorized_videos( current_user: CurrentUserOrDefault, + delete_starred: bool = Query(default=True, description="Whether to allow deletion of starred videos"), ) -> DeleteVideosResult: """Deletes all uncategorized videos owned by the current user (or all if admin). @@ -702,12 +713,15 @@ def delete_uncategorized_videos( deleted_videos: set[str] = set() failed_videos: set[str] = set() affected_boards: set[str] = set() + starred_skipped: set[str] = set() for video_name in names_result.video_names: try: _assert_video_owner(video_name, current_user) - ApiDependencies.invoker.services.videos.delete(video_name) - deleted_videos.add(video_name) - affected_boards.add("none") + if ApiDependencies.invoker.services.videos.delete(video_name, delete_starred=delete_starred): + deleted_videos.add(video_name) + affected_boards.add("none") + else: + starred_skipped.add(video_name) except HTTPException: # Skip videos not owned by the current user — an intentional skip, not a # failed deletion, so it must not be reported (and toasted) as one. @@ -718,6 +732,7 @@ def delete_uncategorized_videos( deleted_videos=list(deleted_videos), failed_videos=list(failed_videos), affected_boards=list(affected_boards), + starred_skipped=list(starred_skipped), ) diff --git a/invokeai/app/services/images/images_base.py b/invokeai/app/services/images/images_base.py index b83eac55b58..7d3018b93ad 100644 --- a/invokeai/app/services/images/images_base.py +++ b/invokeai/app/services/images/images_base.py @@ -155,8 +155,8 @@ def get_many( pass @abstractmethod - def delete(self, image_name: str): - """Deletes an image.""" + def delete(self, image_name: str, delete_starred: bool = True) -> bool: + """Deletes an image, returning ``False`` when a starred image is protected.""" pass @abstractmethod @@ -171,8 +171,13 @@ def delete_intermediates_by_names( pass @abstractmethod - def delete_images_by_names(self, image_names: list[str]) -> tuple[list[str], list[str]]: - """Deletes exactly these images; returns ``(deleted_names, failed_names)``. + def delete_images_by_names( + self, image_names: list[str], delete_starred: bool = True + ) -> tuple[list[str], list[str], list[str]]: + """Deletes exactly these images; returns ``(deleted_names, failed_names, starred_skipped_names)``. + + When ``delete_starred`` is ``False``, starred images are preserved and reported in + ``starred_skipped_names``. For callers that must decide whether the deletion may proceed *before* destroying anything, and so enumerate the names themselves. Same per-image failure semantics as @@ -181,8 +186,10 @@ def delete_images_by_names(self, image_names: list[str]) -> tuple[list[str], lis pass @abstractmethod - def delete_images_on_board(self, board_id: str, user_id: Optional[str] = None) -> tuple[list[str], list[str]]: - """Deletes all images on a board; returns ``(deleted_names, failed_names)``. + def delete_images_on_board( + self, board_id: str, user_id: Optional[str] = None, delete_starred: bool = True + ) -> tuple[list[str], list[str], list[str]]: + """Deletes images on a board; returns ``(deleted_names, failed_names, starred_skipped_names)``. When ``user_id`` is provided, only images owned by that user are deleted (other users' contributions to a public/shared board are preserved). Pass ``None`` for the admin diff --git a/invokeai/app/services/images/images_common.py b/invokeai/app/services/images/images_common.py index 4c96fd825f1..6ddac633b7b 100644 --- a/invokeai/app/services/images/images_common.py +++ b/invokeai/app/services/images/images_common.py @@ -50,6 +50,10 @@ class DeleteImagesResult(ResultWithAffectedBoards): # Required, not defaulted: the client toasts off this field, and an optional one would reach # it as `undefined`. Matches StarredImagesResult. failed_images: list[str] = Field(description="The names of authorized images that could not be deleted") + starred_skipped: list[str] = Field( + default_factory=list, + description="The names of starred images that were skipped because deletion protection was enabled", + ) class StarredImagesResult(ResultWithAffectedBoards): diff --git a/invokeai/app/services/images/images_default.py b/invokeai/app/services/images/images_default.py index d4339f567bf..dd5d13ad940 100644 --- a/invokeai/app/services/images/images_default.py +++ b/invokeai/app/services/images/images_default.py @@ -506,7 +506,7 @@ def get_many( self.__invoker.services.logger.error("Problem getting paginated image DTOs") raise e - def delete(self, image_name: str): + def delete(self, image_name: str, delete_starred: bool = True) -> bool: # Record first, files second, with a durable journal spanning the two. Deleting the record # first means a database failure leaves the image completely intact, and the only state # that can outlive this call is a file nothing references — which the journal lets startup @@ -517,6 +517,8 @@ def delete(self, image_name: str): with self._image_mutation_lock(): try: record = self.__invoker.services.image_records.get(image_name) + if not delete_starred and record.starred: + return False token = self.__invoker.services.image_files.begin_delete([(image_name, record.image_subfolder)]) try: self.__invoker.services.image_records.delete(image_name) @@ -536,6 +538,7 @@ def delete(self, image_name: str): # behind and startup recovery purges the leftover files. self.__invoker.services.logger.error(f"Failed to purge deleted image files: {cleanup_error}") self.notify_deleted(image_name) + return True except ImageRecordNotFoundException: # Already deleted by another request; nothing here failed, so nothing to log. raise @@ -546,7 +549,9 @@ def delete(self, image_name: str): self.__invoker.services.logger.error("Problem deleting image record and file") raise e - def delete_images_on_board(self, board_id: str, user_id: Optional[str] = None) -> tuple[list[str], list[str]]: + def delete_images_on_board( + self, board_id: str, user_id: Optional[str] = None, delete_starred: bool = True + ) -> tuple[list[str], list[str], list[str]]: # The mutation lock spans the enumeration through the purges and rollbacks so a subfolder # move cannot relocate files between a record read and its stage or commit. with self._image_mutation_lock(): @@ -558,10 +563,12 @@ def delete_images_on_board(self, board_id: str, user_id: Optional[str] = None) - is_intermediate=None, user_id=user_id, ) - return self.delete_images_by_names(image_names) + return self.delete_images_by_names(image_names, delete_starred=delete_starred) - def delete_images_by_names(self, image_names: list[str]) -> tuple[list[str], list[str]]: - """Delete exactly these images, returning ``(deleted, failed)``. + def delete_images_by_names( + self, image_names: list[str], delete_starred: bool = True + ) -> tuple[list[str], list[str], list[str]]: + """Delete exactly these images, returning ``(deleted, failed, starred_skipped)``. Split from ``delete_images_on_board`` so a caller that must decide whether the board may go *before* destroying anything can enumerate first and delete second. Records whose file @@ -577,13 +584,16 @@ def delete_images_by_names(self, image_names: list[str]) -> tuple[list[str], lis image_names, StagedMediaDeleteAdapter( kind="image", - stage=lambda name: files.stage_delete(name, image_subfolder=records.get(name).image_subfolder), + load=records.get, + is_starred=lambda record: record.starred, + stage=lambda name, record: files.stage_delete(name, image_subfolder=record.image_subfolder), delete_records=records.delete_many, rollback=files.rollback_delete, commit=files.commit_delete, notify_deleted=self.notify_deleted, log_error=self.__invoker.services.logger.error, ), + delete_starred=delete_starred, ) except ImageFileDeleteException: self.__invoker.services.logger.error("Failed to delete image files") diff --git a/invokeai/app/services/shared/bulk_media_delete.py b/invokeai/app/services/shared/bulk_media_delete.py index a6214b19495..837f5b1a86f 100644 --- a/invokeai/app/services/shared/bulk_media_delete.py +++ b/invokeai/app/services/shared/bulk_media_delete.py @@ -7,12 +7,17 @@ from collections.abc import Callable from dataclasses import dataclass +from typing import Generic, TypeVar + +RecordT = TypeVar("RecordT") @dataclass(frozen=True) -class StagedMediaDeleteAdapter: +class StagedMediaDeleteAdapter(Generic[RecordT]): kind: str - stage: Callable[[str], object] + load: Callable[[str], RecordT] + is_starred: Callable[[RecordT], bool] + stage: Callable[[str, RecordT], object] delete_records: Callable[[list[str]], None] rollback: Callable[[object], None] commit: Callable[[object], None] @@ -20,15 +25,26 @@ class StagedMediaDeleteAdapter: log_error: Callable[[str], None] -def delete_media_by_names(names: list[str], adapter: StagedMediaDeleteAdapter) -> tuple[list[str], list[str]]: - """Delete exactly ``names``, preserving records whenever their files cannot be staged.""" +def delete_media_by_names( + names: list[str], adapter: StagedMediaDeleteAdapter[RecordT], delete_starred: bool = True +) -> tuple[list[str], list[str], list[str]]: + """Delete exactly ``names``, preserving records whenever their files cannot be staged. + + Returns ``(deleted, failed, starred_skipped)``. With ``delete_starred=False`` starred items are + left untouched and reported in ``starred_skipped``. + """ deleted: list[str] = [] failed: list[str] = [] + starred_skipped: list[str] = [] staged: list[tuple[str, object]] = [] for name in names: try: - token = adapter.stage(name) + record = adapter.load(name) + if not delete_starred and adapter.is_starred(record): + starred_skipped.append(name) + continue + token = adapter.stage(name, record) staged.append((name, token)) deleted.append(name) except Exception as error: @@ -54,4 +70,4 @@ def delete_media_by_names(names: list[str], adapter: StagedMediaDeleteAdapter) - for name in deleted: adapter.notify_deleted(name) - return deleted, failed + return deleted, failed, starred_skipped diff --git a/invokeai/app/services/videos/videos_base.py b/invokeai/app/services/videos/videos_base.py index 62f069936c5..cc2a4d6dd34 100644 --- a/invokeai/app/services/videos/videos_base.py +++ b/invokeai/app/services/videos/videos_base.py @@ -144,8 +144,8 @@ def get_many( pass @abstractmethod - def delete(self, video_name: str) -> None: - """Deletes a video.""" + def delete(self, video_name: str, delete_starred: bool = True) -> bool: + """Deletes a video, returning ``False`` when a starred video is protected.""" pass @abstractmethod @@ -160,8 +160,13 @@ def delete_intermediates_by_names( pass @abstractmethod - def delete_videos_by_names(self, video_names: list[str]) -> tuple[list[str], list[str]]: - """Deletes exactly these videos; returns ``(deleted_names, failed_names)``. + def delete_videos_by_names( + self, video_names: list[str], delete_starred: bool = True + ) -> tuple[list[str], list[str], list[str]]: + """Deletes exactly these videos; returns ``(deleted_names, failed_names, starred_skipped_names)``. + + When ``delete_starred`` is ``False``, starred videos are preserved and reported in + ``starred_skipped_names``. For callers that must decide whether the deletion may proceed *before* destroying anything, and so enumerate the names themselves. Same per-video failure semantics as @@ -170,8 +175,10 @@ def delete_videos_by_names(self, video_names: list[str]) -> tuple[list[str], lis pass @abstractmethod - def delete_videos_on_board(self, board_id: str, user_id: Optional[str] = None) -> tuple[list[str], list[str]]: - """Deletes all videos on a board; returns ``(deleted_names, failed_names)``. + def delete_videos_on_board( + self, board_id: str, user_id: Optional[str] = None, delete_starred: bool = True + ) -> tuple[list[str], list[str], list[str]]: + """Deletes videos on a board; returns ``(deleted_names, failed_names, starred_skipped_names)``. When ``user_id`` is provided, only videos owned by that user are deleted (other users' contributions to a public/shared board are preserved). Pass ``None`` for the admin diff --git a/invokeai/app/services/videos/videos_common.py b/invokeai/app/services/videos/videos_common.py index cf5f0384fcc..5a0f41db05d 100644 --- a/invokeai/app/services/videos/videos_common.py +++ b/invokeai/app/services/videos/videos_common.py @@ -44,6 +44,10 @@ class VideoResultWithAffectedBoards(BaseModel): class DeleteVideosResult(VideoResultWithAffectedBoards): deleted_videos: list[str] = Field(description="The names of the videos that were deleted") failed_videos: list[str] = Field(description="The names of videos that were not deleted") + starred_skipped: list[str] = Field( + default_factory=list, + description="The names of starred videos that were skipped because deletion protection was enabled", + ) class StarredVideosResult(VideoResultWithAffectedBoards): diff --git a/invokeai/app/services/videos/videos_default.py b/invokeai/app/services/videos/videos_default.py index 635d878e3ad..b496896d329 100644 --- a/invokeai/app/services/videos/videos_default.py +++ b/invokeai/app/services/videos/videos_default.py @@ -368,12 +368,14 @@ def get_many( self.__invoker.services.logger.error("Problem getting paginated video DTOs") raise e - def delete(self, video_name: str) -> None: + def delete(self, video_name: str, delete_starred: bool = True) -> bool: with self._delete_lock: token: object | None = None record_deleted = False try: record = self.__invoker.services.video_records.get(video_name) + if not delete_starred and record.starred: + return False token = self.__invoker.services.video_files.stage_delete( video_name, video_subfolder=record.video_subfolder ) @@ -384,6 +386,7 @@ def delete(self, video_name: str) -> None: except Exception as cleanup_error: self.__invoker.services.logger.error(f"Failed to purge staged video files: {cleanup_error}") self._on_deleted(video_name) + return True except VideoFileDeleteException: self.__invoker.services.logger.error("Failed to delete video file") raise @@ -396,13 +399,15 @@ def delete(self, video_name: str) -> None: self.__invoker.services.logger.error("Problem deleting video record and file") raise e - def delete_videos_on_board(self, board_id: str, user_id: Optional[str] = None) -> tuple[list[str], list[str]]: + def delete_videos_on_board( + self, board_id: str, user_id: Optional[str] = None, delete_starred: bool = True + ) -> tuple[list[str], list[str], list[str]]: # When ``user_id`` is set the lookup filters to videos owned by that user so the # cascade doesn't destroy other users' contributions to a public/shared board. video_names = self.__invoker.services.board_video_records.get_all_board_video_names_for_board( board_id, categories=None, is_intermediate=None, user_id=user_id ) - return self.delete_videos_by_names(video_names) + return self.delete_videos_by_names(video_names, delete_starred=delete_starred) def delete_intermediates_by_names( self, video_names: list[str], guard: Optional[IntermediateDeleteGuard] = None @@ -427,8 +432,10 @@ def delete_intermediates_by_names( ), ) - def delete_videos_by_names(self, video_names: list[str]) -> tuple[list[str], list[str]]: - """Delete exactly these videos, returning ``(deleted, failed)``. + def delete_videos_by_names( + self, video_names: list[str], delete_starred: bool = True + ) -> tuple[list[str], list[str], list[str]]: + """Delete exactly these videos, returning ``(deleted, failed, starred_skipped)``. Split from ``delete_videos_on_board`` so a caller that must decide whether the board may go *before* destroying anything can enumerate first and delete second. @@ -441,13 +448,16 @@ def delete_videos_by_names(self, video_names: list[str]) -> tuple[list[str], lis video_names, StagedMediaDeleteAdapter( kind="video", - stage=lambda name: files.stage_delete(name, video_subfolder=records.get(name).video_subfolder), + load=records.get, + is_starred=lambda record: record.starred, + stage=lambda name, record: files.stage_delete(name, video_subfolder=record.video_subfolder), delete_records=records.delete_many, rollback=files.rollback_delete, commit=files.commit_delete, notify_deleted=self._on_deleted, log_error=self.__invoker.services.logger.error, ), + delete_starred=delete_starred, ) except VideoFileDeleteException: self.__invoker.services.logger.error("Failed to delete video files") diff --git a/invokeai/frontend/api/openapi.json b/invokeai/frontend/api/openapi.json index 4ab7ee87461..f0d93a8c020 100644 --- a/invokeai/frontend/api/openapi.json +++ b/invokeai/frontend/api/openapi.json @@ -6161,6 +6161,18 @@ "title": "Image Name" }, "description": "The name of the image to delete" + }, + { + "name": "delete_starred", + "in": "query", + "required": false, + "schema": { + "type": "boolean", + "description": "Whether to allow deletion of starred images", + "default": true, + "title": "Delete Starred" + }, + "description": "Whether to allow deletion of starred images" } ], "responses": { @@ -6728,6 +6740,25 @@ "summary": "Delete Uncategorized Images", "description": "Deletes all uncategorized images owned by the current user (or all if admin)", "operationId": "delete_uncategorized_images", + "security": [ + { + "HTTPBearer": [] + } + ], + "parameters": [ + { + "name": "delete_starred", + "in": "query", + "required": false, + "schema": { + "type": "boolean", + "description": "Whether to allow deletion of starred images", + "default": true, + "title": "Delete Starred" + }, + "description": "Whether to allow deletion of starred images" + } + ], "responses": { "200": { "description": "Successful Response", @@ -6738,13 +6769,18 @@ } } } + }, + "422": { + "description": "Validation Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/HTTPValidationError" + } + } + } } - }, - "security": [ - { - "HTTPBearer": [] - } - ] + } } }, "/api/v1/images/copy": { @@ -8452,6 +8488,18 @@ "title": "Video Name" }, "description": "The name of the video to delete" + }, + { + "name": "delete_starred", + "in": "query", + "required": false, + "schema": { + "type": "boolean", + "description": "Whether to allow deletion of starred videos", + "default": true, + "title": "Delete Starred" + }, + "description": "Whether to allow deletion of starred videos" } ], "responses": { @@ -8588,7 +8636,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/VideoNamesBatch" + "$ref": "#/components/schemas/DeleteVideosBatch" } } }, @@ -8629,6 +8677,25 @@ "summary": "Delete Uncategorized Videos", "description": "Deletes all uncategorized videos owned by the current user (or all if admin).\n\nMirrors ``delete_uncategorized_images`` so the \"Delete All Uncategorized\nImages/Videos\" board action covers both media kinds.", "operationId": "delete_uncategorized_videos", + "security": [ + { + "HTTPBearer": [] + } + ], + "parameters": [ + { + "name": "delete_starred", + "in": "query", + "required": false, + "schema": { + "type": "boolean", + "description": "Whether to allow deletion of starred videos", + "default": true, + "title": "Delete Starred" + }, + "description": "Whether to allow deletion of starred videos" + } + ], "responses": { "200": { "description": "Successful Response", @@ -8639,13 +8706,18 @@ } } } + }, + "422": { + "description": "Validation Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/HTTPValidationError" + } + } + } } - }, - "security": [ - { - "HTTPBearer": [] - } - ] + } } }, "/api/v1/videos/i/{video_name}/metadata": { @@ -11019,6 +11091,18 @@ "title": "Include Images" }, "description": "Permanently delete all images and videos on the board" + }, + { + "name": "delete_starred", + "in": "query", + "required": false, + "schema": { + "type": "boolean", + "description": "Whether to allow deletion of starred media", + "default": true, + "title": "Delete Starred" + }, + "description": "Whether to allow deletion of starred media" } ], "responses": { @@ -21809,6 +21893,12 @@ "maxItems": 1000, "title": "Image Names", "description": "The list of names of images to delete" + }, + "delete_starred": { + "type": "boolean", + "title": "Delete Starred", + "description": "Whether to allow deletion of starred images", + "default": true } }, "type": "object", @@ -30884,6 +30974,22 @@ "type": "array", "title": "Failed Videos", "description": "The names of videos that could not be deleted and became uncategorized." + }, + "starred_images_skipped": { + "items": { + "type": "string" + }, + "type": "array", + "title": "Starred Images Skipped", + "description": "The names of starred images that were protected and became uncategorized." + }, + "starred_videos_skipped": { + "items": { + "type": "string" + }, + "type": "array", + "title": "Starred Videos Skipped", + "description": "The names of starred videos that were protected and became uncategorized." } }, "type": "object", @@ -30928,6 +31034,14 @@ "type": "array", "title": "Failed Images", "description": "The names of authorized images that could not be deleted" + }, + "starred_skipped": { + "items": { + "type": "string" + }, + "type": "array", + "title": "Starred Skipped", + "description": "The names of starred images that were skipped because deletion protection was enabled" } }, "type": "object", @@ -30974,6 +31088,29 @@ "title": "DeleteOrphanedModelsResponse", "description": "Response from deleting orphaned models." }, + "DeleteVideosBatch": { + "properties": { + "video_names": { + "items": { + "type": "string", + "maxLength": 255 + }, + "type": "array", + "maxItems": 1000, + "title": "Video Names", + "description": "The list of video names to process" + }, + "delete_starred": { + "type": "boolean", + "title": "Delete Starred", + "description": "Whether to allow deletion of starred videos", + "default": true + } + }, + "type": "object", + "required": ["video_names"], + "title": "DeleteVideosBatch" + }, "DeleteVideosResult": { "properties": { "affected_boards": { @@ -30999,6 +31136,14 @@ "type": "array", "title": "Failed Videos", "description": "The names of videos that were not deleted" + }, + "starred_skipped": { + "items": { + "type": "string" + }, + "type": "array", + "title": "Starred Skipped", + "description": "The names of starred videos that were skipped because deletion protection was enabled" } }, "type": "object", diff --git a/invokeai/frontend/api/schema.ts b/invokeai/frontend/api/schema.ts index 46d86c833be..bb2526ae219 100644 --- a/invokeai/frontend/api/schema.ts +++ b/invokeai/frontend/api/schema.ts @@ -6454,6 +6454,12 @@ export type components = { * @description The list of names of images to delete */ image_names: string[]; + /** + * Delete Starred + * @description Whether to allow deletion of starred images + * @default true + */ + delete_starred?: boolean; }; /** Body_do_hf_login */ Body_do_hf_login: { @@ -11220,6 +11226,16 @@ export type components = { * @description The names of videos that could not be deleted and became uncategorized. */ failed_videos?: string[]; + /** + * Starred Images Skipped + * @description The names of starred images that were protected and became uncategorized. + */ + starred_images_skipped?: string[]; + /** + * Starred Videos Skipped + * @description The names of starred videos that were protected and became uncategorized. + */ + starred_videos_skipped?: string[]; }; /** * DeleteByDestinationResult @@ -11249,6 +11265,11 @@ export type components = { * @description The names of authorized images that could not be deleted */ failed_images: string[]; + /** + * Starred Skipped + * @description The names of starred images that were skipped because deletion protection was enabled + */ + starred_skipped?: string[]; }; /** * DeleteOrphanedModelsRequest @@ -11279,6 +11300,20 @@ export type components = { [key: string]: string; }; }; + /** DeleteVideosBatch */ + DeleteVideosBatch: { + /** + * Video Names + * @description The list of video names to process + */ + video_names: string[]; + /** + * Delete Starred + * @description Whether to allow deletion of starred videos + * @default true + */ + delete_starred?: boolean; + }; /** DeleteVideosResult */ DeleteVideosResult: { /** @@ -11296,6 +11331,11 @@ export type components = { * @description The names of videos that were not deleted */ failed_videos: string[]; + /** + * Starred Skipped + * @description The names of starred videos that were skipped because deletion protection was enabled + */ + starred_skipped?: string[]; }; /** * Denoise - SD1.5, SDXL @@ -55829,7 +55869,10 @@ export interface operations { }; delete_image: { parameters: { - query?: never; + query?: { + /** @description Whether to allow deletion of starred images */ + delete_starred?: boolean; + }; header?: never; path: { /** @description The name of the image to delete */ @@ -56189,7 +56232,10 @@ export interface operations { }; delete_uncategorized_images: { parameters: { - query?: never; + query?: { + /** @description Whether to allow deletion of starred images */ + delete_starred?: boolean; + }; header?: never; path?: never; cookie?: never; @@ -56205,6 +56251,15 @@ export interface operations { "application/json": components["schemas"]["DeleteImagesResult"]; }; }; + /** @description Validation Error */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["HTTPValidationError"]; + }; + }; }; }; copy_images_to_board: { @@ -57073,7 +57128,10 @@ export interface operations { }; delete_video: { parameters: { - query?: never; + query?: { + /** @description Whether to allow deletion of starred videos */ + delete_starred?: boolean; + }; header?: never; path: { /** @description The name of the video to delete */ @@ -57148,7 +57206,7 @@ export interface operations { }; requestBody: { content: { - "application/json": components["schemas"]["VideoNamesBatch"]; + "application/json": components["schemas"]["DeleteVideosBatch"]; }; }; responses: { @@ -57174,7 +57232,10 @@ export interface operations { }; delete_uncategorized_videos: { parameters: { - query?: never; + query?: { + /** @description Whether to allow deletion of starred videos */ + delete_starred?: boolean; + }; header?: never; path?: never; cookie?: never; @@ -57190,6 +57251,15 @@ export interface operations { "application/json": components["schemas"]["DeleteVideosResult"]; }; }; + /** @description Validation Error */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["HTTPValidationError"]; + }; + }; }; }; get_video_metadata: { @@ -58270,6 +58340,8 @@ export interface operations { query?: { /** @description Permanently delete all images and videos on the board */ include_images?: boolean | null; + /** @description Whether to allow deletion of starred media */ + delete_starred?: boolean; }; header?: never; path: { diff --git a/invokeai/frontend/webv2/public/locales/en.json b/invokeai/frontend/webv2/public/locales/en.json index 5ed4de02fc2..664cbae0c5b 100644 --- a/invokeai/frontend/webv2/public/locales/en.json +++ b/invokeai/frontend/webv2/public/locales/en.json @@ -141,6 +141,7 @@ "developerLogEnabled": "Developer Logging", "enableInformationalPopovers": "Informational Popovers", "enableModelDescriptions": "Model Descriptions", + "protectStarredMedia": "Protect Starred Media", "reduceMotion": "Reduce Motion", "showFocusRegionHighlight": "Show Focus Region Highlight", "workflowShowMinimap": "Show Workflow Minimap", @@ -1989,6 +1990,10 @@ "label": "Confirm image deletion", "description": "Ask for confirmation before permanently deleting images." }, + "protectStarredMedia": { + "label": "Protect starred media", + "description": "Deleting from the Gallery skips starred images and videos. Deleting a board with its media moves starred items to Uncategorized instead." + }, "antialiasProgressImages": { "label": "Antialias progress images", "description": "Smooth progress previews instead of rendering them pixelated." @@ -4245,6 +4250,9 @@ "deleteBoardOnly": "Delete Board Only", "deleteBoardPartialTitle": "Deleted board \"{{name}}\" with partial media cleanup", "deleteBoardQuestion": "Delete board \"{{name}}\"?", + "deleteBoardStarredKept": "{{images}} and {{videos}} starred kept in Uncategorized.", + "deleteBoardStarredNotice_one": "Starred media is protected: {{count}} starred item on this board will move to Uncategorized instead of being deleted.", + "deleteBoardStarredNotice_other": "Starred media is protected: {{count}} starred items on this board will move to Uncategorized instead of being deleted.", "deleteBoardSuccessTitle": "Deleted board \"{{name}}\"", "deleteConfirmLabel": "Delete", "deleteConfirmSkip": "Don't ask me again", @@ -4308,6 +4316,10 @@ "failure_one": "Could not delete {{count}} item.", "failure_other": "Could not delete {{count}} items.", "partial": "{{succeeded}} of {{count}} items were deleted; {{failed}} failed.", + "starredKept_one": "{{succeeded}} deleted; {{count}} starred item kept because starred media is protected.", + "starredKept_other": "{{succeeded}} deleted; {{count}} starred items kept because starred media is protected.", + "starredOnly_one": "Starred media is protected; the starred item was not deleted.", + "starredOnly_other": "Starred media is protected; {{count}} starred items were not deleted.", "success_one": "Deleted {{count}} item.", "success_other": "Deleted {{count}} items." }, diff --git a/invokeai/frontend/webv2/src/app/GalleryUiAdapter.tsx b/invokeai/frontend/webv2/src/app/GalleryUiAdapter.tsx index 7f6469b0ecd..58e06f6370d 100644 --- a/invokeai/frontend/webv2/src/app/GalleryUiAdapter.tsx +++ b/invokeai/frontend/webv2/src/app/GalleryUiAdapter.tsx @@ -9,6 +9,7 @@ import { useMountEffect } from '@platform/react/useMountEffect'; import { captureAccountScope, isAccountScopeCurrent } from '@platform/state/accountLifecycle'; import { useProjectLibrarySelector } from '@workbench/projects/library'; import { useExportLibraryProject } from '@workbench/projects/useProjectFileActions'; +import { useWorkbenchPreferenceSelector } from '@workbench/settings/store'; import { useOpenWorkbenchWidget } from '@workbench/useOpenWorkbenchWidget'; import { useLivePreviewFollow } from '@workbench/widgets/preview/livePreviewFollow'; import { getProjectWidgetInstance } from '@workbench/widgetState'; @@ -56,6 +57,7 @@ export const GalleryUiAdapterProvider = ({ children }: { children: ReactNode }) liveFollowEnabled: project.settings.showProgressImagesInViewer, })); const livePreview = useLivePreviewFollow(); + const protectStarredMedia = useWorkbenchPreferenceSelector((preferences) => preferences.protectStarredMedia); const { gallery, notifications, widgets } = useWorkbenchCommands(); const queries = useWorkbenchQueries(); const accountScope = captureAccountScope(); @@ -126,6 +128,7 @@ export const GalleryUiAdapterProvider = ({ children }: { children: ReactNode }) await persistence.ensureProjectOnServer(project); } }, + protectStarredMedia, widgets: { openGallery: () => openWorkbenchWidget('gallery').ok, patchGalleryValues: (values) => widgets.patchValues('gallery', values), @@ -147,6 +150,7 @@ export const GalleryUiAdapterProvider = ({ children }: { children: ReactNode }) projectId, projectName, projects, + protectStarredMedia, queries, store, widgets, diff --git a/invokeai/frontend/webv2/src/features/gallery/core/items.ts b/invokeai/frontend/webv2/src/features/gallery/core/items.ts index ce4b7b0b35d..f8e524565b1 100644 --- a/invokeai/frontend/webv2/src/features/gallery/core/items.ts +++ b/invokeai/frontend/webv2/src/features/gallery/core/items.ts @@ -55,7 +55,10 @@ export interface GalleryItemsPage { export interface GalleryItemMutationResult { /** Boards whose contents/counts changed for at least one confirmed success. */ affectedBoardIds?: string[]; + /** Every requested item that was not changed, `starredSkipped` included. */ failed: GalleryItemRef[]; + /** Starred items a delete left alone because protection was on. They still exist; this is not a failure. */ + starredSkipped?: GalleryItemRef[]; succeeded: GalleryItemRef[]; } diff --git a/invokeai/frontend/webv2/src/features/gallery/core/types.ts b/invokeai/frontend/webv2/src/features/gallery/core/types.ts index c934ea9341d..08c0e426dfc 100644 --- a/invokeai/frontend/webv2/src/features/gallery/core/types.ts +++ b/invokeai/frontend/webv2/src/features/gallery/core/types.ts @@ -97,6 +97,12 @@ export interface GalleryDeletionResult { failedImageNames: string[]; } +/** Options shared by every media deletion. */ +export interface GalleryDeleteOptions { + /** `false` has the backend keep starred media instead of deleting it. Absent means delete everything requested. */ + deleteStarred?: boolean; +} + /** * Authoritative outcome of deleting a board. Deleting the board with its * contents reports deleted/failed media; retaining its contents reports the @@ -110,6 +116,9 @@ export interface GalleryBoardDeletionResult { deletedVideoNames: string[]; failedImageNames: string[]; failedVideoNames: string[]; + /** Starred media kept when protection was on; like the board's other survivors it is now uncategorized. */ + protectedImageNames: string[]; + protectedVideoNames: string[]; } export interface GalleryImagesPage { diff --git a/invokeai/frontend/webv2/src/features/gallery/data/backend.test.ts b/invokeai/frontend/webv2/src/features/gallery/data/backend.test.ts index 52de9f78659..9540d23f11d 100644 --- a/invokeai/frontend/webv2/src/features/gallery/data/backend.test.ts +++ b/invokeai/frontend/webv2/src/features/gallery/data/backend.test.ts @@ -18,6 +18,7 @@ vi.mock('@platform/transport/http', () => ({ import { addImagesToGalleryBoard, + countGalleryBoardStarredItems, createGalleryBoard, deleteGalleryBoard, deleteGalleryImages, @@ -68,6 +69,42 @@ describe('deleteGalleryBoard outcomes', () => { deletedVideoNames: ['gone.mp4'], failedImageNames: ['locked.png'], failedVideoNames: ['locked.mp4'], + protectedImageNames: [], + protectedVideoNames: [], + }); + }); + + it('asks the backend to keep starred media and maps what it kept', async () => { + mocks.apiFetchJson.mockResolvedValue({ + board_id: 'board-1', + deleted_board_images: [], + deleted_board_videos: [], + deleted_images: ['gone.png'], + deleted_videos: [], + failed_images: [], + failed_videos: [], + starred_images_skipped: ['star.png'], + starred_videos_skipped: ['star.mp4'], + }); + + await expect(deleteGalleryBoard('board-1', true, undefined, { deleteStarred: false })).resolves.toMatchObject({ + protectedImageNames: ['star.png'], + protectedVideoNames: ['star.mp4'], + }); + expect(mocks.apiFetchJson).toHaveBeenCalledWith('/api/v1/boards/board-1?delete_starred=false&include_images=true', { + method: 'DELETE', + signal: undefined, + }); + }); + + it('leaves the request unchanged when starred media is not protected', async () => { + mocks.apiFetchJson.mockResolvedValue({ board_id: 'board-1', deleted_board_images: [], deleted_images: [] }); + + await deleteGalleryBoard('board-1', true, undefined, { deleteStarred: true }); + + expect(mocks.apiFetchJson).toHaveBeenCalledWith('/api/v1/boards/board-1?include_images=true', { + method: 'DELETE', + signal: undefined, }); }); @@ -90,6 +127,8 @@ describe('deleteGalleryBoard outcomes', () => { deletedVideoNames: [], failedImageNames: [], failedVideoNames: [], + protectedImageNames: [], + protectedVideoNames: [], }); expect(mocks.apiFetchJson).toHaveBeenCalledWith('/api/v1/boards/board-1?include_images=false', { method: 'DELETE', @@ -1016,3 +1055,32 @@ describe('board membership transport', () => { expect(board).toMatchObject({ id: 'b1', isInbox: true, projectId: 'p1' }); }); }); + +describe('countGalleryBoardStarredItems', () => { + beforeEach(() => { + accountLifecycle.activate('user-a'); + mocks.apiFetchJson.mockReset(); + }); + + it('adds the starred items of both gallery views on the board', async () => { + mocks.apiFetchJson + .mockResolvedValueOnce({ items: [], limit: 0, offset: 0, total: 2 }) + .mockResolvedValueOnce({ items: [], limit: 0, offset: 0, total: 1 }); + + await expect(countGalleryBoardStarredItems('board-1')).resolves.toBe(3); + + const urls = mocks.apiFetchJson.mock.calls.map(([url]) => new URL(String(url), 'https://api.test')); + + expect(urls).toHaveLength(2); + for (const url of urls) { + expect(url.pathname).toBe('/api/v1/gallery/items/'); + expect(url.searchParams.get('limit')).toBe('0'); + expect(url.searchParams.get('board_id')).toBe('board-1'); + expect(url.searchParams.get('starred')).toBe('true'); + } + expect(urls.map((url) => url.searchParams.getAll('categories'))).toEqual([ + ['general'], + ['control', 'mask', 'user'], + ]); + }); +}); diff --git a/invokeai/frontend/webv2/src/features/gallery/data/backend.ts b/invokeai/frontend/webv2/src/features/gallery/data/backend.ts index 388168dba30..6a603beddb5 100644 --- a/invokeai/frontend/webv2/src/features/gallery/data/backend.ts +++ b/invokeai/frontend/webv2/src/features/gallery/data/backend.ts @@ -12,6 +12,7 @@ import type { GalleryBoard, GalleryBoardDeletionResult, GalleryBoardOrderBy, + GalleryDeleteOptions, GalleryDeletionResult, GalleryImage, GalleryImageMetadata, @@ -1135,9 +1136,13 @@ export const updateGalleryBoard = async ( export const deleteGalleryBoard = async ( boardId: string, includeImages: boolean, - signal?: AbortSignal + signal?: AbortSignal, + { deleteStarred }: GalleryDeleteOptions = {} ): Promise => { - const query = toSearchParams({ include_images: includeImages }); + const query = toSearchParams({ + delete_starred: deleteStarred === false ? false : undefined, + include_images: includeImages, + }); const body = await apiFetchJson<{ board_id: string; deleted_board_images: string[]; @@ -1146,6 +1151,8 @@ export const deleteGalleryBoard = async ( deleted_videos?: string[]; failed_images?: string[]; failed_videos?: string[]; + starred_images_skipped?: string[]; + starred_videos_skipped?: string[]; }>(`/api/v1/boards/${encodeURIComponent(boardId)}?${query}`, { method: 'DELETE', signal }); return { @@ -1156,12 +1163,29 @@ export const deleteGalleryBoard = async ( deletedVideoNames: body.deleted_videos ?? [], failedImageNames: body.failed_images ?? [], failedVideoNames: body.failed_videos ?? [], + protectedImageNames: body.starred_images_skipped ?? [], + protectedVideoNames: body.starred_videos_skipped ?? [], }; }; +/** Starred items on a board across both gallery views, so a delete can say what protection will keep. */ +export const countGalleryBoardStarredItems = async (boardId: string, signal?: AbortSignal): Promise => { + const views: GalleryView[] = ['images', 'assets']; + // A zero-row page still reports the total, so no item is serialized for a count. + const pages = await Promise.all( + views.map((galleryView) => + listGalleryItems({ boardId, galleryView, limit: 0, searchTerm: '', signal, starred: true }) + ) + ); + + return pages.reduce((total, page) => total + page.total, 0); +}; + export interface GalleryItemOrganizationTransportResult { affectedBoardIds: string[]; succeededNames: string[]; + /** Starred names a delete left alone because protection was on. They still exist. */ + protectedNames?: string[]; } interface GalleryImageDeleteTransportResult extends GalleryItemOrganizationTransportResult { @@ -1302,7 +1326,8 @@ export const unstarGalleryImages = (imageNames: string[], signal?: AbortSignal): export const deleteGalleryImageItems = async ( imageNames: string[], - signal?: AbortSignal + signal?: AbortSignal, + { deleteStarred }: GalleryDeleteOptions = {} ): Promise => { if (imageNames.length === 0) { return { ...emptyGalleryItemOrganizationTransportResult(), failedNames: [] }; @@ -1310,7 +1335,7 @@ export const deleteGalleryImageItems = async ( signal?.throwIfAborted(); const body = await apiFetchJson('/api/v1/images/delete', { - body: JSON.stringify({ image_names: imageNames }), + body: JSON.stringify({ delete_starred: deleteStarred === false ? false : undefined, image_names: imageNames }), method: 'POST', signal, }); @@ -1319,6 +1344,7 @@ export const deleteGalleryImageItems = async ( return { ...mapGalleryItemOrganizationTransportResult(body, 'deleted_images'), failedNames: getOptionalStringArray(body, 'failed_images'), + protectedNames: getOptionalStringArray(body, 'starred_skipped'), }; }; @@ -1339,7 +1365,8 @@ const mutateGalleryVideoItems = async ( videoNames: string[], operation: 'delete' | 'star' | 'unstar', succeededField: 'deleted_videos' | 'starred_videos' | 'unstarred_videos', - signal?: AbortSignal + signal?: AbortSignal, + extraBody: Record = {} ): Promise => { if (videoNames.length === 0) { return emptyGalleryItemOrganizationTransportResult(); @@ -1347,20 +1374,27 @@ const mutateGalleryVideoItems = async ( signal?.throwIfAborted(); const body = await apiFetchJson(`/api/v1/videos/${operation}`, { - body: JSON.stringify({ video_names: videoNames }), + body: JSON.stringify({ ...extraBody, video_names: videoNames }), method: 'POST', signal, }); signal?.throwIfAborted(); - return mapGalleryVideoOrganizationTransportResult(body, succeededField); + const result = mapGalleryVideoOrganizationTransportResult(body, succeededField); + + return operation === 'delete' + ? { ...result, protectedNames: getOptionalStringArray(body, 'starred_skipped') } + : result; }; export const deleteGalleryVideoItems = ( videoNames: string[], - signal?: AbortSignal + signal?: AbortSignal, + { deleteStarred }: GalleryDeleteOptions = {} ): Promise => - mutateGalleryVideoItems(videoNames, 'delete', 'deleted_videos', signal); + mutateGalleryVideoItems(videoNames, 'delete', 'deleted_videos', signal, { + delete_starred: deleteStarred === false ? false : undefined, + }); export const setGalleryVideoItemsStarred = ( videoNames: string[], diff --git a/invokeai/frontend/webv2/src/features/gallery/data/organization.test.ts b/invokeai/frontend/webv2/src/features/gallery/data/organization.test.ts index e8f5c12ee6c..9d44ea650f8 100644 --- a/invokeai/frontend/webv2/src/features/gallery/data/organization.test.ts +++ b/invokeai/frontend/webv2/src/features/gallery/data/organization.test.ts @@ -70,6 +70,65 @@ describe('galleryItemOrganization transport and confirmed outcomes', () => { }); }); + it('sends the protection flag only when starred media must be kept, and reports the items it kept', async () => { + mocks.apiFetchJson + .mockResolvedValueOnce({ + affected_boards: ['board-a'], + deleted_images: ['plain.png'], + failed_images: [], + starred_skipped: ['star.png'], + }) + .mockResolvedValueOnce({ + affected_boards: [], + deleted_videos: [], + failed_videos: [], + starred_skipped: ['star.mp4'], + }); + + await expect( + galleryItemOrganization.delete([imageRef('plain.png'), imageRef('star.png')], undefined, { + deleteStarred: false, + }) + ).resolves.toEqual({ + affectedBoardIds: ['board-a'], + failed: [imageRef('star.png')], + starredSkipped: [imageRef('star.png')], + succeeded: [imageRef('plain.png')], + }); + await expect( + galleryItemOrganization.delete([videoRef('star.mp4')], undefined, { deleteStarred: false }) + ).resolves.toEqual({ + affectedBoardIds: [], + failed: [videoRef('star.mp4')], + starredSkipped: [videoRef('star.mp4')], + succeeded: [], + }); + + expect(mocks.apiFetchJson).toHaveBeenNthCalledWith(1, '/api/v1/images/delete', { + body: JSON.stringify({ delete_starred: false, image_names: ['plain.png', 'star.png'] }), + method: 'POST', + signal: undefined, + }); + expect(mocks.apiFetchJson).toHaveBeenNthCalledWith(2, '/api/v1/videos/delete', { + body: JSON.stringify({ delete_starred: false, video_names: ['star.mp4'] }), + method: 'POST', + signal: undefined, + }); + }); + + it('ignores a kept name it never asked to delete', async () => { + mocks.apiFetchJson.mockResolvedValue({ + affected_boards: [], + deleted_images: [], + failed_images: [], + starred_skipped: ['other.png'], + }); + + await expect( + galleryItemOrganization.delete([imageRef('asked.png')], undefined, { deleteStarred: false }) + ).resolves.toEqual({ affectedBoardIds: [], failed: [imageRef('asked.png')], succeeded: [] }); + }); + it('uses the video bulk star, unstar, and delete DTOs without inferring success', async () => { mocks.apiFetchJson .mockResolvedValueOnce({ affected_boards: ['board-a'], failed_videos: [], starred_videos: ['clip.mp4'] }) diff --git a/invokeai/frontend/webv2/src/features/gallery/data/queries.ts b/invokeai/frontend/webv2/src/features/gallery/data/queries.ts index cc4cc14ee8f..0a7b0a41354 100644 --- a/invokeai/frontend/webv2/src/features/gallery/data/queries.ts +++ b/invokeai/frontend/webv2/src/features/gallery/data/queries.ts @@ -29,6 +29,7 @@ import { import { type GalleryItemLocation, type GalleryItemNames, + countGalleryBoardStarredItems, fetchImageIndexAvailability, getGalleryItemLocation, hydrateGalleryDateBoardItemPage, @@ -204,6 +205,8 @@ export const galleryKeys = { /** What a video's context menu can offer (recall, embedded workflow), read from its record when opened. */ videoMenuCapabilities: (owner: AccountScope, videoName: string | null) => [...galleryKeys.itemsRoot(), 'menu-capabilities', getAccountKey(owner), videoName] as const, + boardStarredCount: (owner: AccountScope, boardId: string) => + [...galleryKeys.all, 'board-starred-count', getAccountKey(owner), boardId] as const, }; const galleryItemNamesOptionsForOwner = (owner: AccountScope, filter: CanonicalGalleryItemsFilter) => @@ -973,6 +976,27 @@ export const imageIndexAvailabilityOptions = () => { }); }; +/** + * Starred items on one board. Always refetched and never kept: stars change outside the gallery caches, and the + * answer is only read while a delete confirmation is open. + */ +export const galleryBoardStarredCountOptions = (boardId: string) => { + const owner = captureAccountScope(); + + return queryOptions({ + gcTime: 0, + queryFn: async ({ signal }) => { + const count = await countGalleryBoardStarredItems(boardId, AbortSignal.any([signal, owner.signal])); + + assertAccountScopeCurrent(owner); + + return count; + }, + queryKey: galleryKeys.boardStarredCount(owner, boardId), + staleTime: 0, + }); +}; + export const galleryBoardsOptions = (query: GalleryBoardsQuery = {}) => { const owner = captureAccountScope(); const canonicalQuery = canonicalizeBoardsQuery(query); diff --git a/invokeai/frontend/webv2/src/features/gallery/index.ts b/invokeai/frontend/webv2/src/features/gallery/index.ts index c079c029329..2bb352ffb5d 100644 --- a/invokeai/frontend/webv2/src/features/gallery/index.ts +++ b/invokeai/frontend/webv2/src/features/gallery/index.ts @@ -30,6 +30,8 @@ export { type GalleryItemsPage, type GalleryVideoItem, } from './core/items'; +import type { GalleryDeleteOptions } from './core/types'; + import { toGalleryItemKey as getGalleryItemKey, type GalleryItemKind as ItemKind, @@ -145,12 +147,14 @@ const addConfirmedPartition = ({ kind, names, outcome, + protectedKeys, }: { affectedBoardIds: Set; confirmedKeys: Set; kind: ItemKind; names: string[]; outcome: PromiseSettledResult; + protectedKeys: Set; }): void => { if (outcome.status === 'rejected') { return; @@ -159,6 +163,12 @@ const addConfirmedPartition = ({ const requestedNames = new Set(names); let hasConfirmedSuccess = false; + for (const name of outcome.value.protectedNames ?? []) { + if (requestedNames.has(name)) { + protectedKeys.add(getGalleryItemKey({ kind, name })); + } + } + for (const name of outcome.value.succeededNames) { if (!requestedNames.has(name)) { continue; @@ -190,6 +200,7 @@ const mutateGalleryItems = async ( ]); const affectedBoardIds = new Set(); const confirmedKeys = new Set(); + const protectedKeys = new Set(); addConfirmedPartition({ affectedBoardIds, @@ -197,6 +208,7 @@ const mutateGalleryItems = async ( kind: 'image', names: imageNames, outcome: imageOutcome, + protectedKeys, }); addConfirmedPartition({ affectedBoardIds, @@ -204,11 +216,15 @@ const mutateGalleryItems = async ( kind: 'video', names: videoNames, outcome: videoOutcome, + protectedKeys, }); + const starredSkipped = requested.filter((ref) => protectedKeys.has(getGalleryItemKey(ref))); + return { affectedBoardIds: [...affectedBoardIds].sort(), failed: requested.filter((ref) => !confirmedKeys.has(getGalleryItemKey(ref))), + ...(starredSkipped.length > 0 ? { starredSkipped } : {}), succeeded: requested.filter((ref) => confirmedKeys.has(getGalleryItemKey(ref))), }; }; @@ -218,8 +234,17 @@ const mutateGalleryItems = async ( * refs and do not patch caches, invalidate queries, or notify. */ export const galleryItemOrganization = { - delete: (refs: readonly ItemRef[], signal?: AbortSignal): Promise => - mutateGalleryItems(refs, deleteGalleryImageItems, deleteGalleryVideoItems, signal), + delete: ( + refs: readonly ItemRef[], + signal?: AbortSignal, + options?: GalleryDeleteOptions + ): Promise => + mutateGalleryItems( + refs, + (names, requestSignal) => deleteGalleryImageItems(names, requestSignal, options), + (names, requestSignal) => deleteGalleryVideoItems(names, requestSignal, options), + signal + ), moveToBoard: ( refs: readonly ItemRef[], boardId: string, diff --git a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.browser.test.tsx b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.browser.test.tsx index 1b3ebf37b3b..3b11d24f0fd 100644 --- a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.browser.test.tsx +++ b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.browser.test.tsx @@ -2,8 +2,9 @@ import type { GalleryBoard } from '@features/gallery/core/types'; import { ChakraProvider } from '@chakra-ui/react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { system } from '@theme/system'; -import { act } from 'react'; +import { act, type ReactNode } from 'react'; import { createRoot, type Root } from 'react-dom/client'; import { afterEach, beforeEach, expect, it, vi } from 'vitest'; import { userEvent } from 'vitest/browser'; @@ -11,8 +12,19 @@ import { userEvent } from 'vitest/browser'; import type { GalleryWidgetContextValue } from './GalleryWidgetContext'; import { GalleryBoardMenu } from './GalleryBoardMenu'; +import { GalleryUiProvider, type GalleryUiAdapter } from './GalleryUiContext'; import { GalleryWidgetContext } from './GalleryWidgetContext'; +const mocks = vi.hoisted(() => ({ starredCount: 0 })); + +vi.mock('@features/gallery/data/queries', async (importOriginal) => ({ + ...(await importOriginal>()), + galleryBoardStarredCountOptions: (boardId: string) => ({ + queryFn: () => Promise.resolve(mocks.starredCount), + queryKey: ['board-starred-count', boardId], + }), +})); + vi.mock('react-i18next', () => ({ useTranslation: () => ({ t: (key: string, values?: Record) => { @@ -27,6 +39,7 @@ vi.mock('react-i18next', () => ({ 'widgets.gallery.deleteBoardDescription': 'Choose whether the board media moves or is deleted.', 'widgets.gallery.deleteBoardOnly': 'Delete Board Only', 'widgets.gallery.deleteBoardQuestion': `Delete board "${String(values?.name)}"?`, + 'widgets.gallery.deleteBoardStarredNotice': `${String(values?.count)} starred kept`, 'widgets.gallery.downloadBoardWithOmission': `Download Board (${String(values?.count)} video omitted)`, 'widgets.gallery.imageCount': `${String(values?.count)} images`, 'widgets.gallery.videoCount': `${String(values?.count)} videos`, @@ -84,21 +97,50 @@ let host: HTMLDivElement | null = null; let root: Root | null = null; (globalThis as typeof globalThis & { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; +const adapters = { + protected: { protectStarredMedia: true } as unknown as GalleryUiAdapter, + unprotected: { protectStarredMedia: false } as unknown as GalleryUiAdapter, +}; + +const renderMenu = async (widgetContext: GalleryWidgetContextValue, protectStarredMedia = false) => { + const adapter = adapters[protectStarredMedia ? 'protected' : 'unprotected']; + const queryClient = new QueryClient(); + const tree: ReactNode = ( + + + + + + + + + + ); + + await act(async () => { + root?.render(tree); + await Promise.resolve(); + }); +}; + +const openDeleteDialog = async () => { + const deleteItem = Array.from(document.querySelectorAll('[role="menuitem"]')).find( + (element) => element.textContent === 'Delete Board' + ); + + await act(() => userEvent.click(deleteItem as HTMLElement)); + await vi.waitFor(() => { + expect(document.body.textContent).toContain('Delete Board and Media'); + }); +}; + beforeEach(async () => { + mocks.starredCount = 0; host = document.createElement('div'); document.body.append(host); root = createRoot(host); - await act(async () => { - root?.render( - - - - - - ); - await Promise.resolve(); - }); + await renderMenu(context); }); afterEach(async () => { @@ -136,16 +178,7 @@ it('makes the board the auto-add destination', async () => { }); it('hands results back to the selected board from the auto-add board itself', async () => { - await act(async () => { - root?.render( - - - - - - ); - await Promise.resolve(); - }); + await renderMenu(targetContext); const item = document.querySelector('[role="menuitem"][data-value="auto-add-board"]'); @@ -239,3 +272,29 @@ it('leaves an inbox with only the actions its project does not own', async () => expect(labels).not.toContain('Delete Board'); expect(labels).not.toContain('widgets.gallery.renameBoard'); }); + +it('says how many starred items stay when starred media is protected', async () => { + mocks.starredCount = 3; + await renderMenu(context, true); + await openDeleteDialog(); + + await vi.waitFor(() => { + expect(document.body.textContent).toContain('3 starred kept'); + }); +}); + +it('adds nothing to the dialog when starred media is not protected', async () => { + mocks.starredCount = 3; + await renderMenu(context, false); + await openDeleteDialog(); + + expect(document.body.textContent).not.toContain('starred kept'); +}); + +it('adds nothing to the dialog when the board has no starred items', async () => { + await renderMenu(context, true); + await openDeleteDialog(); + await act(() => Promise.resolve()); + + expect(document.body.textContent).not.toContain('starred kept'); +}); diff --git a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.tsx b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.tsx index 30d44ea18ce..b5bd4819777 100644 --- a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.tsx +++ b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.tsx @@ -2,10 +2,12 @@ import type { GalleryBoard } from '@features/gallery/core/types'; import { HStack, Icon, Input, Menu, Portal, Stack, Text } from '@chakra-ui/react'; import { GALLERY_AUTO_ADD_FOLLOW } from '@features/gallery/core/settings'; +import { galleryBoardStarredCountOptions } from '@features/gallery/data/queries'; import { Button } from '@platform/ui/Button'; import { Dialog } from '@platform/ui/Dialog'; import { MenuContent } from '@platform/ui/Menu'; import { MiddleTruncate } from '@platform/ui/MiddleTruncate'; +import { useQuery } from '@tanstack/react-query'; import { ArchiveIcon, ChevronRightIcon, @@ -21,6 +23,7 @@ import { useCallback, useMemo, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { canMoveGalleryBoard, isManagedGalleryBoard } from './galleryBoardGroups'; +import { useOptionalGalleryUi } from './GalleryUiContext'; import { useGalleryWidget } from './GalleryWidgetContext'; export interface GalleryBoardMenuTarget { @@ -42,6 +45,7 @@ export const GalleryBoardMenu = ({ }) => { const { t } = useTranslation(); const { actions, currentUserId, gallery } = useGalleryWidget(); + const protectStarredMedia = useOptionalGalleryUi()?.protectStarredMedia ?? false; const [renameTarget, setRenameTarget] = useState(null); const [deleteTarget, setDeleteTarget] = useState(null); const [renameValue, setRenameValue] = useState(''); @@ -227,6 +231,7 @@ export const GalleryBoardMenu = ({ videos: t('widgets.gallery.videoCount', { count: deleteTarget?.videoCount ?? 0 }), })} + {protectStarredMedia && deleteTarget && } @@ -248,6 +253,22 @@ export const GalleryBoardMenu = ({ ); }; +/** Says how many starred items a "Delete Board and Media" keeps; silent until the count is known and non-zero. */ +const BoardStarredNotice = ({ boardId }: { boardId: string }) => { + const { t } = useTranslation(); + const { data: starredCount } = useQuery(galleryBoardStarredCountOptions(boardId)); + + if (!starredCount) { + return null; + } + + return ( + + {t('widgets.gallery.deleteBoardStarredNotice', { count: starredCount })} + + ); +}; + /** Project export includes the document; the separate media-only download retains its video-omission warning. */ const BoardExportProjectMenuItem = ({ board }: { board: GalleryBoard }) => { const { t } = useTranslation(); diff --git a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryItemSearch.browser.test.tsx b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryItemSearch.browser.test.tsx index 9e948655e88..2b64b5a805f 100644 --- a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryItemSearch.browser.test.tsx +++ b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryItemSearch.browser.test.tsx @@ -80,6 +80,7 @@ const adapter: GalleryUiAdapter = { projectId: 'project-1', projectName: 'Project', projects: [], + protectStarredMedia: false, widgets: { openGallery: () => true, patchGalleryValues: noop }, }; diff --git a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryLoadFailures.browser.test.tsx b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryLoadFailures.browser.test.tsx index 0b72938b623..abe6895e7af 100644 --- a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryLoadFailures.browser.test.tsx +++ b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryLoadFailures.browser.test.tsx @@ -174,6 +174,7 @@ const Harness = ({ progressSessions, projectId: 'project-1', projectName: 'Project', + protectStarredMedia: false, projects: [], widgets: { openGallery: () => true, patchGalleryValues: patch }, }; diff --git a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryUiContext.tsx b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryUiContext.tsx index 442cc675aaa..93d9cdd3636 100644 --- a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryUiContext.tsx +++ b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryUiContext.tsx @@ -124,6 +124,8 @@ export interface GalleryUiAdapter { * saved only after its first debounced flush. Absent for hosts without a workbench, which never create in one. */ ensureProjectOnServer?: () => Promise; + /** Deleting a board with its media keeps starred items, which move to Uncategorized. */ + protectStarredMedia: boolean; /** * Export a project as an `.invk`, reporting progress itself. Keyed by project rather than * board because a board menu can offer this for any project's board, not only the open one. diff --git a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryWidgetView.browser.test.tsx b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryWidgetView.browser.test.tsx index 731cfc2138c..86424f3ef77 100644 --- a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryWidgetView.browser.test.tsx +++ b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryWidgetView.browser.test.tsx @@ -124,6 +124,7 @@ const adapter: GalleryUiAdapter = { progressSessions: [], projectId: 'project-1', projectName: 'Project', + protectStarredMedia: false, projects: [], widgets: { openGallery: () => true, patchGalleryValues: noop }, }; diff --git a/invokeai/frontend/webv2/src/features/gallery/ui/useGalleryActions.browser.test.tsx b/invokeai/frontend/webv2/src/features/gallery/ui/useGalleryActions.browser.test.tsx index b5b5e3cde5d..de640be548b 100644 --- a/invokeai/frontend/webv2/src/features/gallery/ui/useGalleryActions.browser.test.tsx +++ b/invokeai/frontend/webv2/src/features/gallery/ui/useGalleryActions.browser.test.tsx @@ -51,6 +51,9 @@ vi.mock('react-i18next', () => ({ 'widgets.gallery.deleteBoardMoveOutcome': `Moved ${String(values?.images)} and ${String( values?.videos )} to Uncategorized.`, + 'widgets.gallery.deleteBoardStarredKept': `${String(values?.images)} and ${String( + values?.videos + )} starred kept in Uncategorized.`, 'widgets.gallery.deleteBoardPartialTitle': `Deleted board "${String(values?.name)}" with partial media cleanup`, 'widgets.gallery.deleteBoardSuccessTitle': `Deleted board "${String(values?.name)}"`, 'widgets.gallery.downloadReady': 'Download ready', @@ -193,6 +196,7 @@ const adapter: GalleryUiAdapter = { projectName: 'Project', projects: [], ensureProjectOnServer: () => mocks.ensureProjectOnServer(), + protectStarredMedia: false, widgets: { openGallery: () => true, patchGalleryValues }, }; @@ -242,6 +246,8 @@ describe('deleteBoard', () => { deletedVideoNames: [], failedImageNames: [], failedVideoNames: [], + protectedImageNames: [], + protectedVideoNames: [], }; mocks.deleteGalleryBoard.mockResolvedValue(outcome); @@ -267,6 +273,8 @@ describe('deleteBoard', () => { deletedVideoNames: ['one.mp4'], failedImageNames: ['locked.png'], failedVideoNames: ['locked.mp4'], + protectedImageNames: [], + protectedVideoNames: [], }); await act(async () => { @@ -281,6 +289,57 @@ describe('deleteBoard', () => { }); }); +describe('deleteBoard with starred media protection', () => { + const outcome = { + boardId: 'board-1', + deletedBoardImageNames: [], + deletedBoardVideoNames: [], + deletedImageNames: ['plain.png'], + deletedVideoNames: [], + failedImageNames: [], + failedVideoNames: [], + protectedImageNames: ['starred.png', 'starred-too.png'], + protectedVideoNames: ['starred.mp4'], + }; + + afterEach(() => { + adapter.protectStarredMedia = false; + }); + + it('asks the backend to keep starred media and reports what stayed', async () => { + adapter.protectStarredMedia = true; + mocks.deleteGalleryBoard.mockResolvedValue(outcome); + await renderProbe(); + + await act(async () => { + await actionsRef.current?.deleteBoard('board-1', true); + }); + + expect(mocks.deleteGalleryBoard).toHaveBeenCalledWith('board-1', true, expect.any(AbortSignal), { + deleteStarred: false, + }); + expect(reconcileDeletedBoardOutcome).toHaveBeenCalledWith(outcome); + expect(mocks.notificationsAdd).toHaveBeenCalledWith({ + kind: 'success', + message: + 'Deleted 1 images and 0 videos; 0 images and 0 videos failed. 2 images and 1 videos starred kept in Uncategorized.', + title: 'Deleted board "Board 1"', + }); + }); + + it('deletes starred media with the board when protection is off', async () => { + mocks.deleteGalleryBoard.mockResolvedValue({ ...outcome, protectedImageNames: [], protectedVideoNames: [] }); + + await act(async () => { + await actionsRef.current?.deleteBoard('board-1', true); + }); + + expect(mocks.deleteGalleryBoard).toHaveBeenCalledWith('board-1', true, expect.any(AbortSignal), { + deleteStarred: true, + }); + }); +}); + describe('optimistic board updates', () => { it('renames before the request and rolls back when the backend rejects', async () => { mocks.updateGalleryBoard.mockRejectedValue(new Error('rename failed')); diff --git a/invokeai/frontend/webv2/src/features/gallery/ui/useGalleryActions.ts b/invokeai/frontend/webv2/src/features/gallery/ui/useGalleryActions.ts index 4300910b9e8..e6e4f502c46 100644 --- a/invokeai/frontend/webv2/src/features/gallery/ui/useGalleryActions.ts +++ b/invokeai/frontend/webv2/src/features/gallery/ui/useGalleryActions.ts @@ -39,8 +39,16 @@ export const useGalleryActions = ({ loadMore: () => void; selectedBoardId: string; }): GalleryActions => { - const { ensureProjectOnServer, exportProject, gallery, galleryValues, notifications, projectId, widgets } = - useGalleryUi(); + const { + ensureProjectOnServer, + exportProject, + gallery, + galleryValues, + notifications, + projectId, + protectStarredMedia, + widgets, + } = useGalleryUi(); const { autoAddBoardId, showOtherProjectBoards } = getGallerySettings(galleryValues); const queryClient = useQueryClient(); const { t } = useTranslation(); @@ -125,7 +133,9 @@ export const useGalleryActions = ({ try { const boardName = getBoardName(boardId); - const outcome = await deleteGalleryBoard(boardId, includeImages, owner.signal); + const outcome = await deleteGalleryBoard(boardId, includeImages, owner.signal, { + deleteStarred: !protectStarredMedia, + }); assertAccountScopeCurrent(owner); const failedCount = outcome.failedImageNames.length + outcome.failedVideoNames.length; @@ -133,13 +143,24 @@ export const useGalleryActions = ({ failedCount > 0 ? 'widgets.gallery.deleteBoardPartialTitle' : 'widgets.gallery.deleteBoardSuccessTitle', { name: boardName } ); + const protectedCount = outcome.protectedImageNames.length + outcome.protectedVideoNames.length; const message = includeImages - ? t('widgets.gallery.deleteBoardMediaOutcome', { - failedImages: formatImageCount(outcome.failedImageNames.length), - failedVideos: formatVideoCount(outcome.failedVideoNames.length), - images: formatImageCount(outcome.deletedImageNames.length), - videos: formatVideoCount(outcome.deletedVideoNames.length), - }) + ? [ + t('widgets.gallery.deleteBoardMediaOutcome', { + failedImages: formatImageCount(outcome.failedImageNames.length), + failedVideos: formatVideoCount(outcome.failedVideoNames.length), + images: formatImageCount(outcome.deletedImageNames.length), + videos: formatVideoCount(outcome.deletedVideoNames.length), + }), + protectedCount > 0 + ? t('widgets.gallery.deleteBoardStarredKept', { + images: formatImageCount(outcome.protectedImageNames.length), + videos: formatVideoCount(outcome.protectedVideoNames.length), + }) + : null, + ] + .filter(Boolean) + .join(' ') : t('widgets.gallery.deleteBoardMoveOutcome', { images: formatImageCount(outcome.deletedBoardImageNames.length), videos: formatVideoCount(outcome.deletedBoardVideoNames.length), @@ -293,6 +314,7 @@ export const useGalleryActions = ({ loadMore, notifications, projectId, + protectStarredMedia, queryClient, selectedBoardId, showOtherProjectBoards, diff --git a/invokeai/frontend/webv2/src/features/gallery/ui/useGalleryUploadAction.browser.test.tsx b/invokeai/frontend/webv2/src/features/gallery/ui/useGalleryUploadAction.browser.test.tsx index bb55c75f1ad..232afb31fbd 100644 --- a/invokeai/frontend/webv2/src/features/gallery/ui/useGalleryUploadAction.browser.test.tsx +++ b/invokeai/frontend/webv2/src/features/gallery/ui/useGalleryUploadAction.browser.test.tsx @@ -103,6 +103,7 @@ const adapter: GalleryUiAdapter = { projectId: 'project-1', projectName: 'Project', projects: [], + protectStarredMedia: false, widgets: { openGallery: () => true, patchGalleryValues: noop }, }; diff --git a/invokeai/frontend/webv2/src/workbench/WorkbenchRuntime.tsx b/invokeai/frontend/webv2/src/workbench/WorkbenchRuntime.tsx index 1de74b967c7..d99197d2e36 100644 --- a/invokeai/frontend/webv2/src/workbench/WorkbenchRuntime.tsx +++ b/invokeai/frontend/webv2/src/workbench/WorkbenchRuntime.tsx @@ -67,6 +67,8 @@ export const createGalleryBoardReferenceCheck = ( deletedVideoNames: [], failedImageNames: [], failedVideoNames: [], + protectedImageNames: [], + protectedVideoNames: [], }); } }) diff --git a/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.browser.test.tsx b/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.browser.test.tsx index 53351c96bed..6314481ff7a 100644 --- a/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.browser.test.tsx +++ b/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.browser.test.tsx @@ -69,7 +69,7 @@ const mocks = vi.hoisted(() => ({ resolveItem: vi.fn(), setStarred: vi.fn(), })); -const preferences = vi.hoisted(() => ({ confirmImageDeletion: false })); +const preferences = vi.hoisted(() => ({ confirmImageDeletion: false, protectStarredMedia: false })); vi.mock('@features/gallery', () => ({ galleryImages: { @@ -205,6 +205,9 @@ vi.mock('react-i18next', () => ({ if (key === 'widgets.gallery.itemActions.move.success') { return `Moved to ${String(values?.board)}`; } + if (key.startsWith('widgets.gallery.itemActions.delete.starred')) { + return `${key}:${String(values?.count)}:${String(values?.succeeded)}`; + } if (key.startsWith('widgets.gallery.itemActions.loadWorkflow.loadedLabel')) { return `${key}:${String(values?.name)}`; } @@ -284,6 +287,7 @@ const Probe = ({ modelKey = 'sd-1-model', ref }: { modelKey?: string; ref: Ref { vi.clearAllMocks(); preferences.confirmImageDeletion = false; + preferences.protectStarredMedia = false; mocks.requestDeletionConfirmation.mockImplementation( (_itemRefs: readonly GalleryItemRef[], executeDeletion: () => Promise) => executeDeletion() ); @@ -807,7 +811,7 @@ describe('mixed item mutation outcomes', () => { await act(async () => { await executeDeletion?.(); }); - expect(mocks.itemDelete).toHaveBeenCalledWith(refs, expect.any(AbortSignal)); + expect(mocks.itemDelete).toHaveBeenCalledWith(refs, expect.any(AbortSignal), { deleteStarred: true }); }); it('deletes immediately without opening confirmation when the preference is disabled', async () => { @@ -819,7 +823,7 @@ describe('mixed item mutation outcomes', () => { }); expect(mocks.requestDeletionConfirmation).not.toHaveBeenCalled(); - expect(mocks.itemDelete).toHaveBeenCalledWith(refs, expect.any(AbortSignal)); + expect(mocks.itemDelete).toHaveBeenCalledWith(refs, expect.any(AbortSignal), { deleteStarred: true }); }); it('still confirms, naming the surfaces, when the project uses an item and confirmation is off', async () => { @@ -1000,6 +1004,131 @@ describe('mixed item mutation outcomes', () => { }); }); +describe('starred media protection', () => { + const plain = { kind: 'image' as const, name: 'plain.png' }; + const starred = { kind: 'image' as const, name: 'starred.png' }; + const starredVideo = { kind: 'video' as const, name: 'starred.mp4' }; + const rerenderProbe = () => + act(() => { + root?.render( + + + + ); + }); + + beforeEach(async () => { + preferences.protectStarredMedia = true; + mocks.getItemStarred.mockReturnValue( + new Map([ + ['image:starred.png', true], + ['video:starred.mp4', true], + ]) + ); + await rerenderProbe(); + }); + + it('has the backend keep starred items, in case the listing is stale about a star', async () => { + mocks.itemDelete.mockResolvedValue({ affectedBoardIds: ['none'], failed: [], succeeded: [plain] }); + + await act(async () => { + await getItemActions().deleteItems([plain]); + }); + + expect(mocks.itemDelete).toHaveBeenCalledWith([plain], expect.any(AbortSignal), { deleteStarred: false }); + }); + + it('keeps known starred items out of the confirmation and the request, and says how many stayed', async () => { + preferences.confirmImageDeletion = true; + mocks.itemDelete.mockResolvedValue({ affectedBoardIds: ['none'], failed: [], succeeded: [plain] }); + await rerenderProbe(); + + await act(async () => { + await getItemActions().deleteItems([plain, starred, starredVideo]); + }); + + expect(mocks.requestDeletionConfirmation).toHaveBeenCalledWith([plain], expect.any(Function), undefined); + expect(mocks.itemDelete).toHaveBeenCalledWith([plain], expect.any(AbortSignal), { deleteStarred: false }); + expect(mocks.galleryRemoveItems).toHaveBeenCalledWith(['image:plain.png']); + expect(mocks.notificationsAdd).toHaveBeenCalledWith({ + kind: 'info', + title: 'widgets.gallery.itemActions.delete.starredKept:2:1', + }); + }); + + it('only tells the user when everything selected is starred, and sends nothing', async () => { + preferences.confirmImageDeletion = true; + await rerenderProbe(); + + await act(async () => { + await getItemActions().deleteItems([starred, starredVideo]); + }); + + expect(mocks.requestDeletionConfirmation).not.toHaveBeenCalled(); + expect(mocks.itemDelete).not.toHaveBeenCalled(); + expect(mocks.patchGalleryItemCaches).not.toHaveBeenCalled(); + expect(mocks.notificationsAdd).toHaveBeenCalledWith({ + kind: 'info', + title: 'widgets.gallery.itemActions.delete.starredOnly:2:undefined', + }); + }); + + it('stays silent about protection when deletion is not confirmed', async () => { + mocks.itemDelete.mockResolvedValue({ affectedBoardIds: ['none'], failed: [], succeeded: [plain] }); + + await act(async () => { + await getItemActions().deleteItems([starred]); + }); + await act(async () => { + await getItemActions().deleteItems([plain, starred]); + }); + + expect(mocks.itemDelete).toHaveBeenCalledOnce(); + expect(mocks.notificationsAdd).toHaveBeenCalledOnce(); + expect(mocks.notificationsAdd).toHaveBeenCalledWith({ + kind: 'success', + message: undefined, + title: 'widgets.gallery.itemActions.delete.success', + }); + }); + + it('restores an item the backend kept as starred and reports protection rather than failure', async () => { + preferences.confirmImageDeletion = true; + mocks.getItemStarred.mockReturnValue(new Map()); + await rerenderProbe(); + const result = { affectedBoardIds: [], failed: [plain], starredSkipped: [plain], succeeded: [] }; + mocks.itemDelete.mockResolvedValue(result); + + await act(async () => { + await getItemActions().deleteItems([plain]); + }); + + expect(mocks.patchGalleryItemCaches.mock.results[0]?.value).toHaveBeenCalledOnce(); + expect(mocks.reportError).not.toHaveBeenCalled(); + expect(mocks.notificationsAdd).toHaveBeenCalledWith({ + kind: 'info', + title: 'widgets.gallery.itemActions.delete.starredOnly:1:0', + }); + }); + + it('still reports a real failure alongside kept starred items', async () => { + mocks.getItemStarred.mockReturnValue(new Map()); + const locked = { kind: 'image' as const, name: 'locked.png' }; + mocks.itemDelete.mockResolvedValue({ + affectedBoardIds: [], + failed: [plain, locked], + starredSkipped: [plain], + succeeded: [], + }); + + await act(async () => { + await getItemActions().deleteItems([plain, locked]); + }); + + expect(mocks.reportError).toHaveBeenCalledOnce(); + }); +}); + describe('total transport failure rollback', () => { const recentImageFixture = { height: 512, @@ -1093,6 +1222,29 @@ describe('total transport failure rollback', () => { ); }); + it('restores the gallery widget store when the backend keeps every requested item as starred', async () => { + const refs = [{ kind: 'image' as const, name: 'kept.png' }]; + const before = makeMockProject('project-1', { recentImages: [recentImageFixture] }); + const afterRemoval = makeMockProject('project-1', { recentImages: [] }); + + mocks.getSnapshot.mockReturnValueOnce({ activeProject: before, projects: [before] }).mockReturnValue({ + activeProject: afterRemoval, + projects: [afterRemoval], + }); + mocks.itemDelete.mockResolvedValue({ affectedBoardIds: [], failed: refs, starredSkipped: refs, succeeded: [] }); + + await act(async () => { + await getItemActions().deleteItems(refs); + }); + + expect(mocks.galleryWidgetsPatchValues).toHaveBeenCalledWith( + 'gallery', + { recentImages: [recentImageFixture] }, + 'project-1', + 'system' + ); + }); + it('does not clobber a gallery widget field something else changed before the rollback runs', async () => { const refs = [{ kind: 'image' as const, name: 'gone.png' }]; const before = makeMockProject('project-1', { recentImages: [recentImageFixture] }); diff --git a/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.ts b/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.ts index 7bfbf18a13a..abb8c627137 100644 --- a/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.ts +++ b/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.ts @@ -211,6 +211,7 @@ export const useImageActions = ({ const queryClient = useQueryClient(); const { t } = useTranslation(); const confirmImageDeletion = useWorkbenchPreferenceSelector((preferences) => preferences.confirmImageDeletion); + const protectStarredMedia = useWorkbenchPreferenceSelector((preferences) => preferences.protectStarredMedia); const models = useModelsSelector((snapshot) => snapshot.models); const supportedModels = useMemo(() => models.filter(isSupportedGenerateModel), [models]); const vaeModels = useMemo(() => models.filter(isVaeModelConfig).map((model) => model as VaeModelConfig), [models]); @@ -307,15 +308,22 @@ export const useImageActions = ({ action: 'delete' | 'move' | 'star' | 'unstar', requestedCount: number, result: GalleryItemMutationResult, - boardId?: string + boardId?: string, + /** Starred items withheld from the request before it was sent. */ + withheldStarredCount = 0 ) => { - if (result.failed.length > 0) { + // The backend lists the starred items it kept in `failed` too, so that rollback restores them. + const keptStarredCount = result.starredSkipped?.length ?? 0; + const failedCount = result.failed.length - keptStarredCount; + const skippedStarredCount = keptStarredCount + withheldStarredCount; + + if (failedCount > 0) { recordError( new Error( t(`widgets.gallery.itemActions.${action}.${result.succeeded.length > 0 ? 'partial' : 'failure'}`, { board: boardId ? getBoardName(boardId) : undefined, - count: requestedCount, - failed: result.failed.length, + count: requestedCount + withheldStarredCount - skippedStarredCount, + failed: failedCount, succeeded: result.succeeded.length, }) ) @@ -323,6 +331,27 @@ export const useImageActions = ({ return; } + // Protection is a deliberate choice, so it is reported only where deletion is confirmed; otherwise it stays + // silent and the starred items simply remain. + if (skippedStarredCount > 0) { + if (confirmImageDeletion) { + notifications.add({ + kind: 'info', + title: t( + result.succeeded.length > 0 + ? 'widgets.gallery.itemActions.delete.starredKept' + : 'widgets.gallery.itemActions.delete.starredOnly', + { count: skippedStarredCount, succeeded: result.succeeded.length } + ), + }); + return; + } + + if (result.succeeded.length === 0) { + return; + } + } + recordSuccess( t(`widgets.gallery.itemActions.${action}.success`, { board: boardId ? getBoardName(boardId) : undefined, @@ -337,12 +366,15 @@ export const useImageActions = ({ mutate, requested, rollback, + withheldStarredCount, }: { action: 'delete' | 'move' | 'star' | 'unstar'; applyConfirmed: (result: GalleryItemMutationResult, signal: AbortSignal) => Promise | void; boardId?: string; mutate: (signal: AbortSignal) => Promise; requested: GalleryItemRef[]; + /** Starred items a delete left out of `requested` because starred media is protected. */ + withheldStarredCount?: number; /** Undo the optimistic apply. Runs once, only when the account scope that requested * it is still current, before the (likely also-failing) trailing invalidation. */ rollback?: () => void; @@ -383,9 +415,9 @@ export const useImageActions = ({ return; } - reportMutationOutcome(action, requested.length, result, boardId); + reportMutationOutcome(action, requested.length, result, boardId, withheldStarredCount); }; - const deleteItemsConfirmed = (items: GalleryItemRef[]): Promise => { + const deleteItemsConfirmed = (items: GalleryItemRef[], withheldStarredCount = 0): Promise => { // Capture successor context before optimistic removal. Any rejected items restore the widget snapshot; the // confirmed removals then apply again. const deletionContext = getItemActionContext?.() ?? null; @@ -446,6 +478,8 @@ export const useImageActions = ({ } if (result.succeeded.length === 0) { + // Nothing went away (every item was refused or kept), so the widget values cleared up front come back too. + restoreGalleryItemRemoval(galleryWidgetSnapshot); return; } @@ -526,7 +560,8 @@ export const useImageActions = ({ } signal.throwIfAborted(); - return galleryItemOrganization.delete(items, signal); + // The listing the user sees may be stale about stars, so the backend enforces protection too. + return galleryItemOrganization.delete(items, signal, { deleteStarred: !protectStarredMedia }); }, requested: items, rollback: () => { @@ -537,17 +572,55 @@ export const useImageActions = ({ rollbackCachesOnce(); restoreGalleryItemRemoval(galleryWidgetSnapshot); }, + withheldStarredCount, }); }; const deleteItems: ImageActions['deleteItems'] = (items, options) => { + let deletable = items; + + if (protectStarredMedia) { + // Items known to be starred never leave the grid or reach the confirmation; stale knowledge is caught by the + // backend. + const starredByKey = new Map( + [...collectGalleryStoreKnownItemFields(queries.getSnapshot().projects, items)].map(([key, fields]) => [ + key, + fields.starred, + ]) + ); + + for (const [key, cachedStarred] of getGalleryItemStarredFromCaches(queryClient, items)) { + starredByKey.set(key, cachedStarred); + } + + deletable = items.filter((item) => starredByKey.get(toGalleryItemKey(item)) !== true); + } + + const withheldStarredCount = items.length - deletable.length; + + if (deletable.length === 0) { + if (withheldStarredCount > 0 && confirmImageDeletion) { + notifications.add({ + kind: 'info', + title: t('widgets.gallery.itemActions.delete.starredOnly', { count: withheldStarredCount }), + }); + } + + return Promise.resolve(); + } + const project = projectId ? queries.getProject(projectId) : queries.getSnapshot().activeProject; - const usage = project ? collectMediaUsage(project, items) : []; + const usage = project ? collectMediaUsage(project, deletable) : []; // Media the project still uses is always confirmed: the opt-out covers routine deletions, not ones that // leave authored work pointing at nothing. return confirmImageDeletion || usage.length > 0 - ? requestDeletionConfirmation(items, () => deleteItemsConfirmed(items), options?.returnFocus, usage) - : deleteItemsConfirmed(items); + ? requestDeletionConfirmation( + deletable, + () => deleteItemsConfirmed(deletable, withheldStarredCount), + options?.returnFocus, + usage + ) + : deleteItemsConfirmed(deletable, withheldStarredCount); }; const moveItemsToBoard = (items: GalleryItemRef[], boardId: string): Promise => { // On partial move failure, restore then reapply confirmed items. Capture prior boards from cache and store, @@ -1230,6 +1303,7 @@ export const useImageActions = ({ onImagesDeleted, openWorkbenchWidget, projectId, + protectStarredMedia, queryClient, queries, requestDeletionConfirmation, diff --git a/invokeai/frontend/webv2/src/workbench/palette/WorkbenchCommandPalette.pending.browser.test.tsx b/invokeai/frontend/webv2/src/workbench/palette/WorkbenchCommandPalette.pending.browser.test.tsx index ad1dcdd7f70..21f3c37eebf 100644 --- a/invokeai/frontend/webv2/src/workbench/palette/WorkbenchCommandPalette.pending.browser.test.tsx +++ b/invokeai/frontend/webv2/src/workbench/palette/WorkbenchCommandPalette.pending.browser.test.tsx @@ -56,5 +56,5 @@ it('suspends shortcuts from the moment the palette opens, while its module is st }); await expect.poll(() => host.querySelector('[data-testid="palette"]')).not.toBeNull(); // The stand-in steps aside for the loaded dialog, which this test replaces with one that announces nothing. - expect(isModalPresent()).toBe(false); + await expect.poll(() => isModalPresent()).toBe(false); }); diff --git a/invokeai/frontend/webv2/src/workbench/projects/useProjectActions.ts b/invokeai/frontend/webv2/src/workbench/projects/useProjectActions.ts index 22890fac3a8..76f5cb96100 100644 --- a/invokeai/frontend/webv2/src/workbench/projects/useProjectActions.ts +++ b/invokeai/frontend/webv2/src/workbench/projects/useProjectActions.ts @@ -37,6 +37,8 @@ const NO_MEDIA_DELETED: Omit = { deletedVideoNames: [], failedImageNames: [], failedVideoNames: [], + protectedImageNames: [], + protectedVideoNames: [], }; /** diff --git a/invokeai/frontend/webv2/src/workbench/settings/applicationContributions.ts b/invokeai/frontend/webv2/src/workbench/settings/applicationContributions.ts index 76f96611aae..7a2ceab089a 100644 --- a/invokeai/frontend/webv2/src/workbench/settings/applicationContributions.ts +++ b/invokeai/frontend/webv2/src/workbench/settings/applicationContributions.ts @@ -89,6 +89,7 @@ export const projectSettings = section('project', 'Project', [ ]); export const galleryPreferenceSettings = section('gallery', 'Gallery', [ preference('confirmImageDeletion', 'Confirm image deletion', 'delete safety'), + preference('protectStarredMedia', 'Protect starred media', 'delete safety star favorite keep'), ]); export const previewProjectSettings = section('preview', 'Preview', [ { diff --git a/invokeai/frontend/webv2/src/workbench/settings/contracts.ts b/invokeai/frontend/webv2/src/workbench/settings/contracts.ts index 5d130227b74..dcfaa861c00 100644 --- a/invokeai/frontend/webv2/src/workbench/settings/contracts.ts +++ b/invokeai/frontend/webv2/src/workbench/settings/contracts.ts @@ -56,6 +56,8 @@ export interface WorkbenchPreferences { reduceMotion: boolean; showFocusRegionHighlight: boolean; confirmImageDeletion: boolean; + /** Starred images and videos are skipped by every delete, and a board delete moves them to Uncategorized. */ + protectStarredMedia: boolean; /** Auto-switch the Invoke source/destination to match the surface being edited; locks always win. */ autoSwitchInvocationRoute: boolean; queueJobsScope: 'active-project' | 'all'; diff --git a/invokeai/frontend/webv2/src/workbench/settings/store.test.ts b/invokeai/frontend/webv2/src/workbench/settings/store.test.ts index b04734654e1..bd41c4a89ae 100644 --- a/invokeai/frontend/webv2/src/workbench/settings/store.test.ts +++ b/invokeai/frontend/webv2/src/workbench/settings/store.test.ts @@ -387,6 +387,14 @@ describe('normalizeWorkbenchPreferences prompt editing', () => { }); }); +describe('normalizeWorkbenchPreferences starred media protection', () => { + it('is off unless explicitly enabled', () => { + expect(store.normalizeWorkbenchPreferences({}).protectStarredMedia).toBe(false); + expect(store.normalizeWorkbenchPreferences({ protectStarredMedia: 1 as never }).protectStarredMedia).toBe(false); + expect(store.normalizeWorkbenchPreferences({ protectStarredMedia: true }).protectStarredMedia).toBe(true); + }); +}); + describe('normalizeWorkbenchPreferences appearance', () => { it('keeps high contrast off unless explicitly enabled', () => { expect(store.normalizeWorkbenchPreferences({}).highContrast).toBe(false); diff --git a/invokeai/frontend/webv2/src/workbench/settings/store.ts b/invokeai/frontend/webv2/src/workbench/settings/store.ts index 3e13c1dd05b..ded799f8ba0 100644 --- a/invokeai/frontend/webv2/src/workbench/settings/store.ts +++ b/invokeai/frontend/webv2/src/workbench/settings/store.ts @@ -66,6 +66,7 @@ export const DEFAULT_PREFERENCES: WorkbenchPreferences = { notifyOnEnqueue: true, preferNumericAttentionStyle: false, promptFontSize: 'default', + protectStarredMedia: false, queueJobsScope: 'all', reduceMotion: false, showPromptSyntaxHighlighting: true, @@ -337,6 +338,10 @@ export const normalizeWorkbenchPreferences = (preferences?: WorkbenchPreferences promptFontSize: isPromptFontSize(preferences?.promptFontSize) ? preferences.promptFontSize : DEFAULT_PREFERENCES.promptFontSize, + protectStarredMedia: + typeof preferences?.protectStarredMedia === 'boolean' + ? preferences.protectStarredMedia + : DEFAULT_PREFERENCES.protectStarredMedia, queueJobsScope: preferences?.queueJobsScope === 'all-projects' ? 'all' diff --git a/invokeai/frontend/webv2/src/workbench/workbenchState.test.ts b/invokeai/frontend/webv2/src/workbench/workbenchState.test.ts index da47761218a..28810afb50a 100644 --- a/invokeai/frontend/webv2/src/workbench/workbenchState.test.ts +++ b/invokeai/frontend/webv2/src/workbench/workbenchState.test.ts @@ -6058,6 +6058,8 @@ describe('workbenchReducer Phase 5 generation flow', () => { deletedVideoNames: [], failedImageNames: [], failedVideoNames: [], + protectedImageNames: [], + protectedVideoNames: [], }, type: 'reconcileDeletedGalleryBoard', }); @@ -6664,6 +6666,8 @@ describe('workbench backend connection recovery', () => { deletedVideoNames: [deletedVideo.name], failedImageNames: [failedImage.name], failedVideoNames: [failedVideo.name], + protectedImageNames: [], + protectedVideoNames: [], }, type: 'reconcileDeletedGalleryBoard', }); @@ -6720,6 +6724,8 @@ describe('workbench backend connection recovery', () => { deletedVideoNames: [], failedImageNames: [], failedVideoNames: [], + protectedImageNames: [], + protectedVideoNames: [], }, type: 'reconcileDeletedGalleryBoard', }); diff --git a/invokeai/frontend/webv2/src/workbench/workbenchState.ts b/invokeai/frontend/webv2/src/workbench/workbenchState.ts index 4bd67fd72eb..489ed3e2fa8 100644 --- a/invokeai/frontend/webv2/src/workbench/workbenchState.ts +++ b/invokeai/frontend/webv2/src/workbench/workbenchState.ts @@ -5458,6 +5458,8 @@ export const __workbenchReducerInternal = ( ...outcome.deletedBoardVideoNames.map((name) => toGalleryItemKey({ kind: 'video', name })), ...outcome.failedImageNames.map((name) => toGalleryItemKey({ kind: 'image', name })), ...outcome.failedVideoNames.map((name) => toGalleryItemKey({ kind: 'video', name })), + ...outcome.protectedImageNames.map((name) => toGalleryItemKey({ kind: 'image', name })), + ...outcome.protectedVideoNames.map((name) => toGalleryItemKey({ kind: 'video', name })), ]); // Failed and otherwise unconfirmed local items survive. The reconciler diff --git a/tests/app/routers/test_boards_multiuser.py b/tests/app/routers/test_boards_multiuser.py index 47f4e4fb10f..6821d945dce 100644 --- a/tests/app/routers/test_boards_multiuser.py +++ b/tests/app/routers/test_boards_multiuser.py @@ -94,14 +94,14 @@ def enable_multiuser_for_tests(monkeypatch: Any, mock_invoker: Invoker): # ``delete_videos_by_names`` returns the authoritative ``deleted_videos`` list (only the # videos whose file deletion actually succeeded). Default to an empty list so pydantic # validation on ``DeleteBoardResult`` doesn't reject the MagicMock auto-return. - mock_invoker.services.videos.delete_videos_by_names.return_value = ([], []) - mock_invoker.services.videos.delete_videos_on_board.return_value = ([], []) + mock_invoker.services.videos.delete_videos_by_names.return_value = ([], [], []) + mock_invoker.services.videos.delete_videos_on_board.return_value = ([], [], []) # The images service is a real ImageService instance in mock_services; the delete-board # cascade calls ``delete_images_by_names`` on it, which fails without an initialized # invoker. Stub it so the multiuser router tests can assert the cascade args. mock_invoker.services.images = MagicMock() - mock_invoker.services.images.delete_images_by_names.return_value = ([], []) - mock_invoker.services.images.delete_images_on_board.return_value = ([], []) + mock_invoker.services.images.delete_images_by_names.return_value = ([], [], []) + mock_invoker.services.images.delete_images_on_board.return_value = ([], [], []) mock_deps = MockApiDependencies(mock_invoker) monkeypatch.setattr("invokeai.app.api.routers.auth.ApiDependencies", mock_deps) @@ -742,7 +742,7 @@ def test_delete_board_with_include_images_cascades_videos(client: TestClient, mo # The cascade returns the names of videos it actually deleted; the router must surface # *that* list (not the pre-delete enumeration) so the response can't claim a video was # destroyed when its DB record was preserved due to a file-delete failure. - mock_invoker.services.videos.delete_videos_by_names.return_value = (["video_a.mp4", "video_b.mp4"], []) + mock_invoker.services.videos.delete_videos_by_names.return_value = (["video_a.mp4", "video_b.mp4"], [], []) response = client.delete( f"/api/v1/boards/{board_id}?include_images=true", @@ -752,7 +752,9 @@ def test_delete_board_with_include_images_cascades_videos(client: TestClient, mo body = response.json() assert body["board_id"] == board_id assert set(body["deleted_videos"]) == {"video_a.mp4", "video_b.mp4"} - mock_invoker.services.videos.delete_videos_by_names.assert_called_once_with(["video_a.mp4", "video_b.mp4"]) + mock_invoker.services.videos.delete_videos_by_names.assert_called_once_with( + ["video_a.mp4", "video_b.mp4"], delete_starred=True + ) def test_delete_board_with_partial_video_file_delete_failure_reports_only_actual_deletes( @@ -775,8 +777,8 @@ def test_delete_board_with_partial_video_file_delete_failure_reports_only_actual # The service deleted "good.mp4" successfully but preserved "stuck.mp4" because its # file delete failed. The router must NOT claim "stuck.mp4" was deleted, and must # report the failure from the service's own accounting (not a racy listing diff). - mock_invoker.services.images.delete_images_by_names.return_value = (["good.png"], ["stuck.png"]) - mock_invoker.services.videos.delete_videos_by_names.return_value = (["good.mp4"], ["stuck.mp4"]) + mock_invoker.services.images.delete_images_by_names.return_value = (["good.png"], ["stuck.png"], []) + mock_invoker.services.videos.delete_videos_by_names.return_value = (["good.mp4"], ["stuck.mp4"], []) response = client.delete( f"/api/v1/boards/{board_id}?include_images=true", @@ -790,6 +792,51 @@ def test_delete_board_with_partial_video_file_delete_failure_reports_only_actual assert body["failed_videos"] == ["stuck.mp4"] +def test_delete_board_preserves_starred_media_when_protected( + client: TestClient, mock_invoker: Invoker, user1_token: str +): + create = client.post( + "/api/v1/boards/?board_name=Protected+Starred+Board", + headers={"Authorization": f"Bearer {user1_token}"}, + ) + assert create.status_code == status.HTTP_201_CREATED + board_id = create.json()["board_id"] + mock_invoker.services.images.delete_images_by_names.return_value = (["normal.png"], [], ["starred.png"]) + mock_invoker.services.videos.delete_videos_by_names.return_value = (["normal.mp4"], [], ["starred.mp4"]) + + response = client.delete( + f"/api/v1/boards/{board_id}?include_images=true&delete_starred=false", + headers={"Authorization": f"Bearer {user1_token}"}, + ) + + assert response.status_code == status.HTTP_200_OK + body = response.json() + assert body["deleted_images"] == ["normal.png"] + assert body["deleted_videos"] == ["normal.mp4"] + assert body["starred_images_skipped"] == ["starred.png"] + assert body["starred_videos_skipped"] == ["starred.mp4"] + assert mock_invoker.services.images.delete_images_by_names.call_args.kwargs["delete_starred"] is False + assert mock_invoker.services.videos.delete_videos_by_names.call_args.kwargs["delete_starred"] is False + + +def test_delete_board_deletes_starred_media_by_default(client: TestClient, mock_invoker: Invoker, user1_token: str): + create = client.post( + "/api/v1/boards/?board_name=Unprotected+Board", + headers={"Authorization": f"Bearer {user1_token}"}, + ) + assert create.status_code == status.HTTP_201_CREATED + board_id = create.json()["board_id"] + + response = client.delete( + f"/api/v1/boards/{board_id}?include_images=true", + headers={"Authorization": f"Bearer {user1_token}"}, + ) + + assert response.status_code == status.HTTP_200_OK + assert mock_invoker.services.images.delete_images_by_names.call_args.kwargs["delete_starred"] is True + assert mock_invoker.services.videos.delete_videos_by_names.call_args.kwargs["delete_starred"] is True + + @pytest.mark.parametrize("failure_phase", ["videos", "board"]) def test_delete_board_reports_partial_cascade_completion( client: TestClient, mock_invoker: Invoker, user1_token: str, failure_phase: str @@ -805,8 +852,8 @@ def test_delete_board_reports_partial_cascade_completion( ) assert create.status_code == status.HTTP_201_CREATED board_id = create.json()["board_id"] - mock_invoker.services.images.delete_images_by_names.return_value = (["deleted.png"], []) - mock_invoker.services.videos.delete_videos_by_names.return_value = (["deleted.mp4"], []) + mock_invoker.services.images.delete_images_by_names.return_value = (["deleted.png"], [], []) + mock_invoker.services.videos.delete_videos_by_names.return_value = (["deleted.mp4"], [], []) if failure_phase == "videos": mock_invoker.services.videos.delete_videos_by_names.side_effect = RuntimeError("video delete failed") @@ -1069,7 +1116,7 @@ def test_a_member_board_deletes_like_any_other( headers={"Authorization": f"Bearer {user1_token}"}, ).json() mock_invoker.services.board_images.get_all_board_image_names_for_board.return_value = ["member.png"] - mock_invoker.services.images.delete_images_by_names.return_value = (["member.png"], []) + mock_invoker.services.images.delete_images_by_names.return_value = (["member.png"], [], []) response = client.delete( f"/api/v1/boards/{member['board_id']}?include_images={'true' if include_images else 'false'}", @@ -1078,7 +1125,7 @@ def test_a_member_board_deletes_like_any_other( assert response.status_code == status.HTTP_200_OK if include_images: - mock_invoker.services.images.delete_images_by_names.assert_called_once_with(["member.png"]) + mock_invoker.services.images.delete_images_by_names.assert_called_once_with(["member.png"], delete_starred=True) assert response.json()["deleted_images"] == ["member.png"] else: mock_invoker.services.images.delete_images_by_names.assert_not_called() diff --git a/tests/app/routers/test_images.py b/tests/app/routers/test_images.py index b07d76ed357..bdcf5a06e90 100644 --- a/tests/app/routers/test_images.py +++ b/tests/app/routers/test_images.py @@ -97,6 +97,133 @@ def prepare_image_maintenance_test(monkeypatch: Any, mock_invoker: Invoker) -> N monkeypatch.setattr("invokeai.app.api.auth_dependencies.ApiDependencies", mock_deps) +def prepare_starred_delete_test( + monkeypatch: Any, + mock_invoker: Invoker, + starred_names: set[str], + board_ids: dict[str, str | None], +) -> None: + mock_deps = MockApiDependencies(mock_invoker) + mock_invoker.services.image_moves = MagicMock() + mock_invoker.services.image_moves.is_maintenance_active.return_value = False + mock_invoker.services.board_images = MagicMock() + mock_invoker.services.images = MagicMock() + mock_invoker.services.images.get_dto.side_effect = lambda image_name: MagicMock(board_id=board_ids[image_name]) + mock_invoker.services.images.delete.side_effect = ( + lambda image_name, delete_starred=True: delete_starred or image_name not in starred_names + ) + monkeypatch.setattr("invokeai.app.api.routers.images.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.routers._access.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.routers.image_move_maintenance.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.auth_dependencies.ApiDependencies", mock_deps) + + +def test_delete_starred_image_is_skipped_when_protected( + monkeypatch: Any, mock_invoker: Invoker, client: TestClient +) -> None: + image_name = "starred.png" + prepare_starred_delete_test( + monkeypatch, mock_invoker, starred_names={image_name}, board_ids={image_name: "board-id"} + ) + + response = client.delete(f"/api/v1/images/i/{image_name}", params={"delete_starred": False}) + + assert response.status_code == 200 + assert response.json() == { + "deleted_images": [], + "failed_images": [], + "affected_boards": [], + "starred_skipped": [image_name], + } + mock_invoker.services.images.delete.assert_called_once_with(image_name, delete_starred=False) + + +def test_delete_starred_image_is_deleted_by_default( + monkeypatch: Any, mock_invoker: Invoker, client: TestClient +) -> None: + image_name = "starred.png" + prepare_starred_delete_test( + monkeypatch, mock_invoker, starred_names={image_name}, board_ids={image_name: "board-id"} + ) + + response = client.delete(f"/api/v1/images/i/{image_name}") + + assert response.status_code == 200 + assert response.json()["deleted_images"] == [image_name] + assert response.json()["starred_skipped"] == [] + mock_invoker.services.images.delete.assert_called_once_with(image_name, delete_starred=True) + + +def test_bulk_delete_only_deletes_unstarred_images_when_protected( + monkeypatch: Any, mock_invoker: Invoker, client: TestClient +) -> None: + starred_name = "starred.png" + unstarred_name = "unstarred.png" + prepare_starred_delete_test( + monkeypatch, + mock_invoker, + starred_names={starred_name}, + board_ids={starred_name: "board-id", unstarred_name: "board-id"}, + ) + + response = client.post( + "/api/v1/images/delete", + json={"image_names": [starred_name, unstarred_name], "delete_starred": False}, + ) + + assert response.status_code == 200 + assert response.json() == { + "deleted_images": [unstarred_name], + "failed_images": [], + "affected_boards": ["board-id"], + "starred_skipped": [starred_name], + } + mock_invoker.services.images.delete.assert_any_call(starred_name, delete_starred=False) + mock_invoker.services.images.delete.assert_any_call(unstarred_name, delete_starred=False) + + +def test_bulk_delete_without_flag_keeps_deleting_starred_images( + monkeypatch: Any, mock_invoker: Invoker, client: TestClient +) -> None: + starred_name = "starred.png" + prepare_starred_delete_test( + monkeypatch, mock_invoker, starred_names={starred_name}, board_ids={starred_name: "board-id"} + ) + + response = client.post("/api/v1/images/delete", json={"image_names": [starred_name]}) + + assert response.status_code == 200 + assert response.json()["deleted_images"] == [starred_name] + assert response.json()["starred_skipped"] == [] + + +def test_delete_uncategorized_only_deletes_unstarred_images_when_protected( + monkeypatch: Any, mock_invoker: Invoker, client: TestClient +) -> None: + starred_name = "starred.png" + unstarred_name = "unstarred.png" + prepare_starred_delete_test( + monkeypatch, + mock_invoker, + starred_names={starred_name}, + board_ids={starred_name: None, unstarred_name: None}, + ) + mock_invoker.services.board_images.get_all_board_image_names_for_board.return_value = [ + starred_name, + unstarred_name, + ] + + response = client.delete("/api/v1/images/uncategorized", params={"delete_starred": False}) + + assert response.status_code == 200 + assert response.json() == { + "deleted_images": [unstarred_name], + "failed_images": [], + "affected_boards": ["none"], + "starred_skipped": [starred_name], + } + + @pytest.mark.parametrize( ("method", "path", "json_body"), [ @@ -672,9 +799,11 @@ def get_dto(image_name: str) -> MagicMock: dto.board_id = "board-2" if image_name == "vanished.png" else "board-1" return dto - def delete(image_name: str) -> None: + def delete(image_name: str, delete_starred: bool = True) -> bool: + del delete_starred if image_name == "vanished.png" and raise_from == "delete": raise ImageRecordNotFoundException + return True images_service.get_dto.side_effect = get_dto images_service.delete.side_effect = delete @@ -737,9 +866,11 @@ def test_delete_still_reports_a_genuine_storage_failure( dto.board_id = "board-1" images_service.get_dto.return_value = dto - def delete(image_name: str) -> None: + def delete(image_name: str, delete_starred: bool = True) -> bool: + del delete_starred if image_name == "broken.png": raise RuntimeError("storage is on fire") + return True images_service.delete.side_effect = delete diff --git a/tests/app/routers/test_multiuser_authorization.py b/tests/app/routers/test_multiuser_authorization.py index 76f32f43065..c5ed0003c71 100644 --- a/tests/app/routers/test_multiuser_authorization.py +++ b/tests/app/routers/test_multiuser_authorization.py @@ -1400,7 +1400,7 @@ def test_delete_uncategorized_reports_owned_images_that_failed( monkeypatch.setattr( mock_invoker.services.images, "delete", - MagicMock(side_effect=[None, OSError("file busy")]), + MagicMock(side_effect=[True, OSError("file busy")]), ) response = client.delete("/api/v1/images/uncategorized", headers=_auth(user1_token)) diff --git a/tests/app/routers/test_videos_multiuser.py b/tests/app/routers/test_videos_multiuser.py index a72d10cc51b..743c591ad8f 100644 --- a/tests/app/routers/test_videos_multiuser.py +++ b/tests/app/routers/test_videos_multiuser.py @@ -366,6 +366,73 @@ def test_vanished_board_still_reads_as_an_ordinary_refusal_for_videos( assert response.status_code == status.HTTP_403_FORBIDDEN +def test_delete_starred_video_is_skipped_when_protected(client: TestClient, mock_invoker: Invoker, admin_token: str): + fake_dto = MagicMock() + fake_dto.board_id = "board-id" + mock_invoker.services.videos.get_dto.return_value = fake_dto + mock_invoker.services.videos.delete.return_value = False + + response = client.delete( + "/api/v1/videos/i/starred.mp4?delete_starred=false", + headers={"Authorization": f"Bearer {admin_token}"}, + ) + + assert response.status_code == status.HTTP_200_OK + assert response.json() == { + "deleted_videos": [], + "failed_videos": [], + "affected_boards": [], + "starred_skipped": ["starred.mp4"], + } + mock_invoker.services.videos.delete.assert_called_once_with("starred.mp4", delete_starred=False) + + +def test_bulk_delete_only_deletes_unstarred_videos_when_protected( + client: TestClient, mock_invoker: Invoker, admin_token: str +): + fake_dto = MagicMock() + fake_dto.board_id = None + mock_invoker.services.videos.get_dto.return_value = fake_dto + mock_invoker.services.videos.delete.side_effect = ( + lambda video_name, delete_starred=True: delete_starred or video_name != "starred.mp4" + ) + + response = client.post( + "/api/v1/videos/delete", + json={"video_names": ["starred.mp4", "normal.mp4"], "delete_starred": False}, + headers={"Authorization": f"Bearer {admin_token}"}, + ) + + assert response.status_code == status.HTTP_200_OK + assert response.json() == { + "deleted_videos": ["normal.mp4"], + "failed_videos": [], + "affected_boards": ["none"], + "starred_skipped": ["starred.mp4"], + } + + +def test_bulk_delete_without_flag_keeps_deleting_starred_videos( + client: TestClient, mock_invoker: Invoker, admin_token: str +): + fake_dto = MagicMock() + fake_dto.board_id = None + mock_invoker.services.videos.get_dto.return_value = fake_dto + mock_invoker.services.videos.delete.side_effect = ( + lambda video_name, delete_starred=True: delete_starred or video_name != "starred.mp4" + ) + + response = client.post( + "/api/v1/videos/delete", + json={"video_names": ["starred.mp4"]}, + headers={"Authorization": f"Bearer {admin_token}"}, + ) + + assert response.status_code == status.HTTP_200_OK + assert response.json()["deleted_videos"] == ["starred.mp4"] + assert response.json()["starred_skipped"] == [] + + def test_video_batch_rejects_too_many_or_overlong_names() -> None: with pytest.raises(ValidationError): VideoNamesBatch(video_names=[f"{index}.mp4" for index in range(1001)]) @@ -1173,6 +1240,30 @@ def test_delete_uncategorized_videos_deletes_only_owned(client: TestClient, mock assert delete_calls == {"mine_a.mp4", "mine_b.mp4"} +def test_delete_uncategorized_videos_preserves_starred_when_protected( + client: TestClient, mock_invoker: Invoker, admin_token: str +): + names_result = MagicMock() + names_result.video_names = ["starred.mp4", "normal.mp4"] + mock_invoker.services.videos.get_video_names.return_value = names_result + mock_invoker.services.videos.delete.side_effect = ( + lambda video_name, delete_starred=True: delete_starred or video_name != "starred.mp4" + ) + + response = client.delete( + "/api/v1/videos/uncategorized?delete_starred=false", + headers={"Authorization": f"Bearer {admin_token}"}, + ) + + assert response.status_code == status.HTTP_200_OK + assert response.json() == { + "deleted_videos": ["normal.mp4"], + "failed_videos": [], + "affected_boards": ["none"], + "starred_skipped": ["starred.mp4"], + } + + def test_delete_uncategorized_videos_requires_auth(enable_multiuser_for_videos: Any, client: TestClient): response = client.delete("/api/v1/videos/uncategorized") assert response.status_code == status.HTTP_401_UNAUTHORIZED diff --git a/tests/app/services/images/test_images_default.py b/tests/app/services/images/test_images_default.py index 1a4424d9fd4..7fd545d20b0 100644 --- a/tests/app/services/images/test_images_default.py +++ b/tests/app/services/images/test_images_default.py @@ -60,6 +60,7 @@ def _make_record( image_name: str = "abc12345-test.png", image_subfolder: str = "", is_intermediate: bool = False, + starred: bool = False, ) -> ImageRecord: now = get_iso_timestamp() return ImageRecord( @@ -71,7 +72,7 @@ def _make_record( created_at=now, updated_at=now, is_intermediate=is_intermediate, - starred=False, + starred=starred, has_workflow=False, image_subfolder=image_subfolder, ) @@ -325,7 +326,7 @@ def test_record_preserved_when_file_delete_fails(self, image_service: ImageServi # File staging succeeds for first, fails for second invoker.services.image_files.stage_delete.side_effect = [object(), Exception("disk error")] - deleted, failed = image_service.delete_images_on_board("board-1") + deleted, failed, starred_skipped = image_service.delete_images_on_board("board-1") invoker.services.image_records.delete_many.assert_called_once_with(["good.png"]) assert deleted == ["good.png"] @@ -340,7 +341,7 @@ def test_file_cleanup_failure_does_not_raise(self, image_service: ImageService): invoker.services.image_records.get.return_value = record invoker.services.image_files.stage_delete.side_effect = Exception("permission denied") - deleted, failed = image_service.delete_images_on_board("board-1") + deleted, failed, starred_skipped = image_service.delete_images_on_board("board-1") invoker.services.image_records.delete_many.assert_called_once_with([]) assert deleted == [] @@ -357,7 +358,7 @@ def test_record_lookup_failure_does_not_block_others(self, image_service: ImageS ok_record = _make_record(image_name="ok.png", image_subfolder="") invoker.services.image_records.get.side_effect = [Exception("not found"), ok_record] - deleted, failed = image_service.delete_images_on_board("board-1") + deleted, failed, starred_skipped = image_service.delete_images_on_board("board-1") # File staging was attempted for the second image only invoker.services.image_files.stage_delete.assert_called_once_with("ok.png", image_subfolder="") @@ -1145,7 +1146,7 @@ def competitor_rolls_back_then_delete(image_names: list[str]) -> None: monkeypatch.setattr(records, "delete_many", competitor_rolls_back_then_delete) - deleted, failed = svc.delete_images_on_board("board-1") + deleted, failed, starred_skipped = svc.delete_images_on_board("board-1") assert deleted == ["img.png"] assert failed == [] @@ -1479,3 +1480,104 @@ def test_a_failed_measurement_leaves_the_size_unknown_and_the_image_intact(self, assert dto.file_size_bytes is None assert storage.get_path("made.png").exists() + + +class TestStarredProtection: + def test_starred_image_is_deleted_by_default(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_records.get.return_value = _make_record(starred=True) + + assert image_service.delete("starred.png") is True + + invoker.services.image_files.begin_delete.assert_called_once() + invoker.services.image_records.delete.assert_called_once_with("starred.png") + invoker.services.image_files.commit_delete.assert_called_once() + + def test_starred_image_is_preserved_when_protected(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_records.get.return_value = _make_record(starred=True) + + assert image_service.delete("starred.png", delete_starred=False) is False + + invoker.services.image_files.begin_delete.assert_not_called() + invoker.services.image_records.delete.assert_not_called() + invoker.services.image_files.commit_delete.assert_not_called() + + def test_unstarred_image_is_deleted_when_protected(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_records.get.return_value = _make_record(starred=False) + + assert image_service.delete("normal.png", delete_starred=False) is True + + invoker.services.image_records.delete.assert_called_once_with("normal.png") + + def test_board_delete_reports_protected_starred_images(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.board_image_records.get_all_board_image_names_for_board.return_value = [ + "starred.png", + "normal.png", + ] + invoker.services.image_records.get.side_effect = [ + _make_record(image_name="starred.png", starred=True), + _make_record(image_name="normal.png", starred=False), + ] + + deleted, failed, starred_skipped = image_service.delete_images_on_board("board-1", delete_starred=False) + + assert deleted == ["normal.png"] + assert failed == [] + assert starred_skipped == ["starred.png"] + invoker.services.image_records.delete_many.assert_called_once_with(["normal.png"]) + invoker.services.image_files.stage_delete.assert_called_once_with("normal.png", image_subfolder="") + + def test_board_delete_removes_starred_images_by_default(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.board_image_records.get_all_board_image_names_for_board.return_value = ["starred.png"] + invoker.services.image_records.get.return_value = _make_record(image_name="starred.png", starred=True) + + deleted, failed, starred_skipped = image_service.delete_images_on_board("board-1") + + assert (deleted, failed, starred_skipped) == (["starred.png"], [], []) + + +class TestStarredProtectionAgainstRealStores: + def _seed(self, wired, name: str, *, starred: bool) -> None: + svc, records, storage = wired + _seed_record(records, name, is_intermediate=False) + _save_image_file(storage, name) + if starred: + records.update(name, ImageRecordChanges(starred=True)) + + def test_protected_batch_removes_unstarred_rows_and_files_and_keeps_starred_ones_whole(self, wired) -> None: + svc, records, storage = wired + self._seed(wired, "plain.png", starred=False) + self._seed(wired, "star.png", starred=True) + + deleted, failed, starred_skipped = svc.delete_images_by_names(["plain.png", "star.png"], delete_starred=False) + + assert (deleted, failed, starred_skipped) == (["plain.png"], [], ["star.png"]) + with pytest.raises(ImageRecordNotFoundException): + records.get("plain.png") + assert not storage.get_path("plain.png").exists() + assert not storage.get_path("plain.png", thumbnail=True).exists() + assert records.get("star.png").starred is True + assert storage.get_path("star.png").exists() + assert storage.get_path("star.png", thumbnail=True).exists() + assert _staging_dirs(storage) == [] + + def test_unprotected_batch_still_deletes_starred_images(self, wired) -> None: + svc, records, storage = wired + self._seed(wired, "star.png", starred=True) + + assert svc.delete_images_by_names(["star.png"]) == (["star.png"], [], []) + assert not storage.get_path("star.png").exists() + + def test_protected_single_delete_leaves_a_starred_image_and_no_staging_behind(self, wired) -> None: + svc, records, storage = wired + self._seed(wired, "star.png", starred=True) + + assert svc.delete("star.png", delete_starred=False) is False + + assert records.get("star.png").starred is True + assert storage.get_path("star.png").exists() + assert _staging_dirs(storage) == [] diff --git a/tests/app/services/intermediates/test_intermediates_service.py b/tests/app/services/intermediates/test_intermediates_service.py index 2f2b838c214..d4c5ec62d9f 100644 --- a/tests/app/services/intermediates/test_intermediates_service.py +++ b/tests/app/services/intermediates/test_intermediates_service.py @@ -1518,7 +1518,7 @@ def pause_thumbnail_move(path, target): def delete_video(): if bulk: - assert invoker.services.videos.delete_videos_by_names(["racy.mp4"]) == (["racy.mp4"], []) + assert invoker.services.videos.delete_videos_by_names(["racy.mp4"]) == (["racy.mp4"], [], []) else: invoker.services.videos.delete("racy.mp4") diff --git a/tests/app/services/shared/test_bulk_media_delete.py b/tests/app/services/shared/test_bulk_media_delete.py new file mode 100644 index 00000000000..aabe8a56e95 --- /dev/null +++ b/tests/app/services/shared/test_bulk_media_delete.py @@ -0,0 +1,93 @@ +from dataclasses import dataclass +from unittest.mock import MagicMock + +import pytest + +from invokeai.app.services.shared.bulk_media_delete import StagedMediaDeleteAdapter, delete_media_by_names + + +@dataclass +class _Record: + starred: bool = False + + +def _adapter( + records: dict[str, _Record], + *, + stage_failures: frozenset[str] = frozenset(), + delete_records: MagicMock | None = None, +) -> tuple[StagedMediaDeleteAdapter[_Record], MagicMock, MagicMock, MagicMock]: + rollback = MagicMock() + commit = MagicMock() + notify = MagicMock() + + def stage(name: str, record: _Record) -> object: + if name in stage_failures: + raise OSError("device busy") + return f"token:{name}" + + adapter = StagedMediaDeleteAdapter( + kind="image", + load=records.__getitem__, + is_starred=lambda record: record.starred, + stage=stage, + delete_records=delete_records or MagicMock(), + rollback=rollback, + commit=commit, + notify_deleted=notify, + log_error=MagicMock(), + ) + return adapter, rollback, commit, notify + + +RECORDS = {"plain.png": _Record(), "star.png": _Record(starred=True), "other.png": _Record()} + + +def test_starred_names_are_deleted_unless_protection_is_requested() -> None: + adapter, _, _, _ = _adapter(dict(RECORDS)) + + assert delete_media_by_names(["plain.png", "star.png"], adapter) == (["plain.png", "star.png"], [], []) + + +def test_protection_skips_starred_names_without_staging_or_deleting_them() -> None: + delete_records = MagicMock() + adapter, rollback, commit, notify = _adapter(dict(RECORDS), delete_records=delete_records) + + result = delete_media_by_names(["plain.png", "star.png"], adapter, delete_starred=False) + + assert result == (["plain.png"], [], ["star.png"]) + delete_records.assert_called_once_with(["plain.png"]) + commit.assert_called_once_with("token:plain.png") + notify.assert_called_once_with("plain.png") + rollback.assert_not_called() + + +def test_protection_with_only_starred_names_deletes_no_records() -> None: + delete_records = MagicMock() + adapter, _, commit, notify = _adapter(dict(RECORDS), delete_records=delete_records) + + assert delete_media_by_names(["star.png"], adapter, delete_starred=False) == ([], [], ["star.png"]) + + delete_records.assert_called_once_with([]) + commit.assert_not_called() + notify.assert_not_called() + + +def test_a_failed_stage_is_reported_failed_not_starred_and_protection_still_applies_to_the_rest() -> None: + adapter, _, _, _ = _adapter(dict(RECORDS), stage_failures=frozenset({"other.png"})) + + result = delete_media_by_names(["plain.png", "other.png", "star.png"], adapter, delete_starred=False) + + assert result == (["plain.png"], ["other.png"], ["star.png"]) + + +def test_a_record_delete_failure_restores_only_the_staged_items() -> None: + delete_records = MagicMock(side_effect=RuntimeError("database is locked")) + adapter, rollback, commit, notify = _adapter(dict(RECORDS), delete_records=delete_records) + + with pytest.raises(RuntimeError): + delete_media_by_names(["plain.png", "star.png"], adapter, delete_starred=False) + + rollback.assert_called_once_with("token:plain.png") + commit.assert_not_called() + notify.assert_not_called() diff --git a/tests/app/services/videos/test_videos_default.py b/tests/app/services/videos/test_videos_default.py index ebfd3fe2d6e..4a57fa93b4c 100644 --- a/tests/app/services/videos/test_videos_default.py +++ b/tests/app/services/videos/test_videos_default.py @@ -15,7 +15,7 @@ from invokeai.app.util.misc import get_iso_timestamp -def _make_record(video_name: str = "abc.mp4", video_subfolder: str = "") -> VideoRecord: +def _make_record(video_name: str = "abc.mp4", video_subfolder: str = "", starred: bool = False) -> VideoRecord: now = get_iso_timestamp() return VideoRecord( video_name=video_name, @@ -28,7 +28,7 @@ def _make_record(video_name: str = "abc.mp4", video_subfolder: str = "") -> Vide created_at=now, updated_at=now, is_intermediate=False, - starred=False, + starred=starred, has_workflow=False, video_subfolder=video_subfolder, ) @@ -60,7 +60,7 @@ def test_record_preserved_when_file_delete_fails(self, video_service: VideoServi ] invoker.services.video_files.stage_delete.side_effect = [object(), Exception("disk error")] - deleted, failed = video_service.delete_videos_on_board("board-1") + deleted, failed, starred_skipped = video_service.delete_videos_on_board("board-1") # Only the video whose file we successfully removed should have its record deleted. invoker.services.video_records.delete_many.assert_called_once_with(["good.mp4"]) @@ -80,7 +80,7 @@ def test_file_cleanup_failure_does_not_raise(self, video_service: VideoService): invoker.services.video_files.stage_delete.side_effect = Exception("permission denied") # Should not raise - deleted, failed = video_service.delete_videos_on_board("board-1") + deleted, failed, starred_skipped = video_service.delete_videos_on_board("board-1") # And the failing video's record must be preserved. invoker.services.video_records.delete_many.assert_called_once_with([]) @@ -99,7 +99,7 @@ def test_all_records_deleted_on_full_success(self, video_service: VideoService): ] invoker.services.video_files.stage_delete.side_effect = [object(), object()] - deleted, failed = video_service.delete_videos_on_board("board-1") + deleted, failed, starred_skipped = video_service.delete_videos_on_board("board-1") invoker.services.video_records.delete_many.assert_called_once_with(["a.mp4", "b.mp4"]) assert deleted == ["a.mp4", "b.mp4"] @@ -112,7 +112,7 @@ def test_staging_cleanup_failure_is_deferred_after_records_are_deleted(self, vid invoker.services.video_files.stage_delete.return_value = object() invoker.services.video_files.commit_delete.side_effect = OSError("staging directory busy") - deleted, failed = video_service.delete_videos_on_board("board-1") + deleted, failed, starred_skipped = video_service.delete_videos_on_board("board-1") assert deleted == ["v.mp4"] assert failed == [] @@ -192,6 +192,63 @@ def test_copy_preserves_provenance_and_removes_an_identity_that_missed_its_board assert create_args["move_source"] is False +class TestStarredProtection: + def test_starred_video_is_deleted_by_default(self, video_service: VideoService): + invoker = video_service._VideoService__invoker # type: ignore[attr-defined] + invoker.services.video_records.get.return_value = _make_record(starred=True) + + assert video_service.delete("starred.mp4") is True + + invoker.services.video_files.stage_delete.assert_called_once_with("starred.mp4", video_subfolder="") + invoker.services.video_records.delete.assert_called_once_with("starred.mp4") + invoker.services.video_files.commit_delete.assert_called_once() + + def test_starred_video_is_preserved_when_protected(self, video_service: VideoService): + invoker = video_service._VideoService__invoker # type: ignore[attr-defined] + invoker.services.video_records.get.return_value = _make_record(starred=True) + + assert video_service.delete("starred.mp4", delete_starred=False) is False + + invoker.services.video_files.stage_delete.assert_not_called() + invoker.services.video_records.delete.assert_not_called() + + def test_unstarred_video_is_deleted_when_protected(self, video_service: VideoService): + invoker = video_service._VideoService__invoker # type: ignore[attr-defined] + invoker.services.video_records.get.return_value = _make_record(starred=False) + + assert video_service.delete("normal.mp4", delete_starred=False) is True + + invoker.services.video_records.delete.assert_called_once_with("normal.mp4") + + def test_board_delete_reports_protected_starred_videos(self, video_service: VideoService): + invoker = video_service._VideoService__invoker # type: ignore[attr-defined] + invoker.services.board_video_records.get_all_board_video_names_for_board.return_value = [ + "starred.mp4", + "normal.mp4", + ] + invoker.services.video_records.get.side_effect = [ + _make_record(video_name="starred.mp4", starred=True), + _make_record(video_name="normal.mp4", starred=False), + ] + + deleted, failed, starred_skipped = video_service.delete_videos_on_board("board-1", delete_starred=False) + + assert deleted == ["normal.mp4"] + assert failed == [] + assert starred_skipped == ["starred.mp4"] + invoker.services.video_records.delete_many.assert_called_once_with(["normal.mp4"]) + invoker.services.video_files.stage_delete.assert_called_once_with("normal.mp4", video_subfolder="") + + def test_board_delete_removes_starred_videos_by_default(self, video_service: VideoService): + invoker = video_service._VideoService__invoker # type: ignore[attr-defined] + invoker.services.board_video_records.get_all_board_video_names_for_board.return_value = ["starred.mp4"] + invoker.services.video_records.get.return_value = _make_record(video_name="starred.mp4", starred=True) + + deleted, failed, starred_skipped = video_service.delete_videos_on_board("board-1") + + assert (deleted, failed, starred_skipped) == (["starred.mp4"], [], []) + + class TestCreateRollback: """Per JPPhoto's PR review (May 22 follow-up): if the video file save fails after the DB record has been written, the create path must roll back the record (and any board From 29c1b123cf24c02bd36577504143be3632552439 Mon Sep 17 00:00:00 2001 From: FloeHetling Date: Wed, 14 Oct 2026 00:03:17 +0300 Subject: [PATCH 2/2] fix(gallery): make starred protection atomic with record deletion Protected deletes now refuse starred rows in the same transaction that removes the records, so a star set while files were being staged keeps its record and its files are restored. Applies to single, batch and board deletes of images and videos. Starred-only deletes without confirmation now say nothing was deleted; the board dialog notice names the button it applies to and handles loading and errors. --- .../Users Guide/06.Gallery/introduction.mdx | 2 +- .../image_records/image_records_base.py | 8 ++++ .../image_records/image_records_default.py | 3 ++ .../app/services/images/images_default.py | 23 +++++++-- .../app/services/shared/bulk_media_delete.py | 23 +++++++-- .../shared/database/queries/images.py | 20 ++++++++ .../shared/database/queries/videos.py | 20 ++++++++ .../video_records/video_records_base.py | 8 ++++ .../video_records/video_records_default.py | 3 ++ .../app/services/videos/videos_default.py | 8 +++- .../frontend/webv2/public/locales/en.json | 5 +- .../ui/GalleryBoardMenu.browser.test.tsx | 18 ++++++- .../features/gallery/ui/GalleryBoardMenu.tsx | 17 ++++--- .../useImageActions.browser.test.tsx | 47 +++++++++++++++++-- .../image-actions/useImageActions.ts | 39 +++++++-------- .../image_records/test_image_records.py | 4 +- .../services/images/test_images_default.py | 44 ++++++++++++++++- .../services/shared/test_bulk_media_delete.py | 30 ++++++++---- .../video_records/test_video_records.py | 4 +- .../services/videos/test_videos_default.py | 16 ++++++- 20 files changed, 279 insertions(+), 63 deletions(-) diff --git a/docs/src/content/docs/Users Guide/06.Gallery/introduction.mdx b/docs/src/content/docs/Users Guide/06.Gallery/introduction.mdx index 2bb90edef56..97f70bad963 100644 --- a/docs/src/content/docs/Users Guide/06.Gallery/introduction.mdx +++ b/docs/src/content/docs/Users Guide/06.Gallery/introduction.mdx @@ -150,7 +150,7 @@ Drag items onto a board in the board list; if the list is hidden, it opens while Press **Delete** (or **Backspace**), choose **Delete Image**, **Delete Video** or **Delete Selection** from the menu, or use the delete button in the selection bar. Deleting permanently removes the items from every board and from disk, and cannot be undone. A confirmation asks first. Tick **Don't ask me again** and confirm to stop asking for ordinary deletions (cancelling leaves it on); you can turn it back on, or off, with **Confirm image deletion** in Settings under Gallery. -To keep starred items safe from accidental deletion, turn on **Protect starred media** in the **Gallery** section of Settings. While it is on, deleting skips starred images and videos: from a mixed selection only the unstarred items are deleted, and when everything selected is starred nothing is deleted. If **Confirm image deletion** is on, a notice says how many starred items were kept; otherwise they are kept silently. Unstar an item to delete it. The setting is off by default. +To keep starred items safe from accidental deletion, turn on **Protect starred media** in the **Gallery** section of Settings. While it is on, deleting skips starred images and videos: from a mixed selection only the unstarred items are deleted, and when everything selected is starred nothing is deleted. If **Confirm image deletion** is on, a notice says how many starred items were kept; with it off you only get a notice when nothing could be deleted because everything selected was starred. Unstar an item to delete it. The setting is off by default. When the open project still uses something you are deleting, for example as a canvas layer, a reference image, an upscale or video input, or in a workflow, the confirmation always appears and lists where, even if you turned confirmations off. Only the open project's current work is checked: other projects, undo history and canvas snapshots are not, so check before you delete media you still need there. If deleting is interrupted, InvokeAI cleans up any leftover files the next time it starts. diff --git a/invokeai/app/services/image_records/image_records_base.py b/invokeai/app/services/image_records/image_records_base.py index 541c66e8cfd..e581d3dc08b 100644 --- a/invokeai/app/services/image_records/image_records_base.py +++ b/invokeai/app/services/image_records/image_records_base.py @@ -89,6 +89,14 @@ def delete_many(self, image_names: list[str]) -> None: """Deletes many image records.""" pass + @abstractmethod + def delete_many_keeping_starred(self, image_names: list[str]) -> list[str]: + """Deletes the named image records that are not starred, on one transaction with the starred check. + + Returns the names kept because they are starred, including ones starred after the caller last looked. + """ + pass + @abstractmethod def get_subfolders(self, image_names: list[str]) -> dict[str, str]: """Maps each existing named image to its on-disk subfolder; absent names are omitted.""" diff --git a/invokeai/app/services/image_records/image_records_default.py b/invokeai/app/services/image_records/image_records_default.py index 86a8c29e0ea..72bf5b1ee8c 100644 --- a/invokeai/app/services/image_records/image_records_default.py +++ b/invokeai/app/services/image_records/image_records_default.py @@ -100,6 +100,9 @@ def delete(self, image_name: str) -> None: def delete_many(self, image_names: list[str]) -> None: self._queries.images.delete_many(image_names) + def delete_many_keeping_starred(self, image_names: list[str]) -> list[str]: + return self._queries.images.delete_many_keeping_starred(image_names) + def iter_all_image_locations(self, batch_size: int = 500) -> Iterator[tuple[str, str]]: """Yields every image location through bounded keyset pages, including hidden/intermediate rows.""" if batch_size <= 0: diff --git a/invokeai/app/services/images/images_default.py b/invokeai/app/services/images/images_default.py index dd5d13ad940..189361b6a20 100644 --- a/invokeai/app/services/images/images_default.py +++ b/invokeai/app/services/images/images_default.py @@ -520,17 +520,31 @@ def delete(self, image_name: str, delete_starred: bool = True) -> bool: if not delete_starred and record.starred: return False token = self.__invoker.services.image_files.begin_delete([(image_name, record.image_subfolder)]) - try: - self.__invoker.services.image_records.delete(image_name) - except Exception: - # The image is still live: drop the journal and leave its files alone. + + def abandon_journal() -> None: try: self.__invoker.services.image_files.abandon_delete(token) except Exception as cleanup_error: self.__invoker.services.logger.error( f"Failed to discard the delete journal for {image_name}: {cleanup_error}" ) + + try: + if delete_starred: + self.__invoker.services.image_records.delete(image_name) + starred_since = False + else: + # The starred check is part of the delete, so a star that arrives after the read above wins. + starred_since = bool( + self.__invoker.services.image_records.delete_many_keeping_starred([image_name]) + ) + except Exception: + # The image is still live: drop the journal and leave its files alone. + abandon_journal() raise + if starred_since: + abandon_journal() + return False try: self.__invoker.services.image_files.commit_delete(token) except Exception as cleanup_error: @@ -588,6 +602,7 @@ def delete_images_by_names( is_starred=lambda record: record.starred, stage=lambda name, record: files.stage_delete(name, image_subfolder=record.image_subfolder), delete_records=records.delete_many, + delete_unstarred_records=records.delete_many_keeping_starred, rollback=files.rollback_delete, commit=files.commit_delete, notify_deleted=self.notify_deleted, diff --git a/invokeai/app/services/shared/bulk_media_delete.py b/invokeai/app/services/shared/bulk_media_delete.py index 837f5b1a86f..92f1cff6dfd 100644 --- a/invokeai/app/services/shared/bulk_media_delete.py +++ b/invokeai/app/services/shared/bulk_media_delete.py @@ -1,8 +1,10 @@ """Transactional coordination for deleting batches of media records and files. Images and videos have different stores but the same invariant: files are staged first, records -are deleted only for successful stages, and every stage rolls back if the record batch fails. The -adapter is the seam between that policy and the two storage implementations. +are deleted only for successful stages, and every stage rolls back if the record batch fails. When +starred media is protected, the record delete itself refuses starred rows, so a star that lands +after the check still keeps its record, and the files staged for it are restored. The adapter is +the seam between that policy and the two storage implementations. """ from collections.abc import Callable @@ -19,6 +21,7 @@ class StagedMediaDeleteAdapter(Generic[RecordT]): is_starred: Callable[[RecordT], bool] stage: Callable[[str, RecordT], object] delete_records: Callable[[list[str]], None] + delete_unstarred_records: Callable[[list[str]], list[str]] rollback: Callable[[object], None] commit: Callable[[object], None] notify_deleted: Callable[[str], None] @@ -31,7 +34,7 @@ def delete_media_by_names( """Delete exactly ``names``, preserving records whenever their files cannot be staged. Returns ``(deleted, failed, starred_skipped)``. With ``delete_starred=False`` starred items are - left untouched and reported in ``starred_skipped``. + left untouched and reported in ``starred_skipped``, including ones starred after they were read. """ deleted: list[str] = [] failed: list[str] = [] @@ -52,7 +55,19 @@ def delete_media_by_names( adapter.log_error(f"Failed to delete {adapter.kind} file {name}; keeping record: {error}") try: - adapter.delete_records(deleted) + if delete_starred: + adapter.delete_records(deleted) + else: + # The starred check and the delete share one transaction, so only a row nobody starred meanwhile goes. + kept_names = set(adapter.delete_unstarred_records(deleted)) + for name, token in [entry for entry in staged if entry[0] in kept_names]: + try: + adapter.rollback(token) + except Exception as rollback_error: + adapter.log_error(f"Failed to restore starred {adapter.kind} files for {name}: {rollback_error}") + staged = [entry for entry in staged if entry[0] not in kept_names] + starred_skipped.extend(name for name in deleted if name in kept_names) + deleted = [name for name in deleted if name not in kept_names] except Exception: for name, token in staged: try: diff --git a/invokeai/app/services/shared/database/queries/images.py b/invokeai/app/services/shared/database/queries/images.py index 4ac4580dec6..6f0b2aca4cc 100644 --- a/invokeai/app/services/shared/database/queries/images.py +++ b/invokeai/app/services/shared/database/queries/images.py @@ -102,6 +102,15 @@ .limit(bindparam("limit")) ) _DELETE_MANY = delete(images).where(_I.image_name.in_(bindparam("image_names", expanding=True))) +# Locked, so that an image starred meanwhile is either seen starred and kept, or deleted before it is. +_LOCK_STARRED = ( + select(_I.image_name) + .where(_I.image_name.in_(bindparam("image_names", expanding=True)), _I.starred == true()) + .with_for_update() +) +_DELETE_UNSTARRED = delete(images).where( + _I.image_name.in_(bindparam("image_names", expanding=True)), _I.starred == false() +) # Locked, so that an image promoted out of the intermediates meanwhile is either seen promoted or deleted before it is. _LOCK_INTERMEDIATES = ( select(_I.image_name) @@ -516,6 +525,17 @@ def delete_many(self, conn: Connection, image_names: Sequence[str]) -> None: for chunk in itertools.batched(image_names, IN_CHUNK): conn.execute(_DELETE_MANY, {"image_names": list(chunk)}) + @write + def delete_many_keeping_starred(self, conn: Connection, image_names: Sequence[str]) -> list[str]: + """Deletes those of the named images that are not starred; the names kept because they are starred.""" + kept: list[str] = [] + for chunk in itertools.batched(image_names, IN_CHUNK): + names = list(chunk) + starred: set[str] = set(conn.execute(_LOCK_STARRED, {"image_names": names}).scalars().all()) + conn.execute(_DELETE_UNSTARRED, {"image_names": names}) + kept.extend(name for name in names if name in starred) + return kept + @write def delete_intermediates(self, conn: Connection, image_names: Sequence[str]) -> list[str]: """Deletes those of the named images that are intermediates; the names deleted, in the given order.""" diff --git a/invokeai/app/services/shared/database/queries/videos.py b/invokeai/app/services/shared/database/queries/videos.py index 6e049974741..a5155f187ce 100644 --- a/invokeai/app/services/shared/database/queries/videos.py +++ b/invokeai/app/services/shared/database/queries/videos.py @@ -95,6 +95,15 @@ ) _DELETE = delete(videos).where(_V.video_name == bindparam("video_name")) _DELETE_MANY = delete(videos).where(_V.video_name.in_(bindparam("video_names", expanding=True))) +# Locked, so that a video starred meanwhile is either seen starred and kept, or deleted before it is. +_LOCK_STARRED = ( + select(_V.video_name) + .where(_V.video_name.in_(bindparam("video_names", expanding=True)), _V.starred == true()) + .with_for_update() +) +_DELETE_UNSTARRED = delete(videos).where( + _V.video_name.in_(bindparam("video_names", expanding=True)), _V.starred == false() +) # Locked, so that a video promoted out of the intermediates meanwhile is either seen promoted or deleted before it is. _LOCK_INTERMEDIATES = ( select(_V.video_name) @@ -455,6 +464,17 @@ def delete_many(self, conn: Connection, video_names: Sequence[str]) -> None: for chunk in itertools.batched(video_names, IN_CHUNK): conn.execute(_DELETE_MANY, {"video_names": list(chunk)}) + @write + def delete_many_keeping_starred(self, conn: Connection, video_names: Sequence[str]) -> list[str]: + """Deletes those of the named videos that are not starred; the names kept because they are starred.""" + kept: list[str] = [] + for chunk in itertools.batched(video_names, IN_CHUNK): + names = list(chunk) + starred: set[str] = set(conn.execute(_LOCK_STARRED, {"video_names": names}).scalars().all()) + conn.execute(_DELETE_UNSTARRED, {"video_names": names}) + kept.extend(name for name in names if name in starred) + return kept + @write def delete_intermediates(self, conn: Connection, video_names: Sequence[str]) -> list[str]: """Deletes those of the named videos that are intermediates; the names deleted, in the given order.""" diff --git a/invokeai/app/services/video_records/video_records_base.py b/invokeai/app/services/video_records/video_records_base.py index 5be03be4e01..18e17c6e70b 100644 --- a/invokeai/app/services/video_records/video_records_base.py +++ b/invokeai/app/services/video_records/video_records_base.py @@ -86,6 +86,14 @@ def delete_many(self, video_names: list[str]) -> None: """Deletes many video records.""" pass + @abstractmethod + def delete_many_keeping_starred(self, video_names: list[str]) -> list[str]: + """Deletes the named video records that are not starred, on one transaction with the starred check. + + Returns the names kept because they are starred, including ones starred after the caller last looked. + """ + pass + @abstractmethod def save( self, diff --git a/invokeai/app/services/video_records/video_records_default.py b/invokeai/app/services/video_records/video_records_default.py index 4ec0a7a3b8a..281970c704a 100644 --- a/invokeai/app/services/video_records/video_records_default.py +++ b/invokeai/app/services/video_records/video_records_default.py @@ -108,6 +108,9 @@ def delete(self, video_name: str) -> None: def delete_many(self, video_names: list[str]) -> None: self._queries.videos.delete_many(video_names) + def delete_many_keeping_starred(self, video_names: list[str]) -> list[str]: + return self._queries.videos.delete_many_keeping_starred(video_names) + def save( self, video_name: str, diff --git a/invokeai/app/services/videos/videos_default.py b/invokeai/app/services/videos/videos_default.py index b496896d329..1488d341951 100644 --- a/invokeai/app/services/videos/videos_default.py +++ b/invokeai/app/services/videos/videos_default.py @@ -379,7 +379,12 @@ def delete(self, video_name: str, delete_starred: bool = True) -> bool: token = self.__invoker.services.video_files.stage_delete( video_name, video_subfolder=record.video_subfolder ) - self.__invoker.services.video_records.delete(video_name) + if delete_starred: + self.__invoker.services.video_records.delete(video_name) + elif self.__invoker.services.video_records.delete_many_keeping_starred([video_name]): + # Starred after the read above: the delete refused the row, so the files go back. + self.__invoker.services.video_files.rollback_delete(token) + return False record_deleted = True try: self.__invoker.services.video_files.commit_delete(token) @@ -452,6 +457,7 @@ def delete_videos_by_names( is_starred=lambda record: record.starred, stage=lambda name, record: files.stage_delete(name, video_subfolder=record.video_subfolder), delete_records=records.delete_many, + delete_unstarred_records=records.delete_many_keeping_starred, rollback=files.rollback_delete, commit=files.commit_delete, notify_deleted=self._on_deleted, diff --git a/invokeai/frontend/webv2/public/locales/en.json b/invokeai/frontend/webv2/public/locales/en.json index 664cbae0c5b..9312004694b 100644 --- a/invokeai/frontend/webv2/public/locales/en.json +++ b/invokeai/frontend/webv2/public/locales/en.json @@ -4251,8 +4251,9 @@ "deleteBoardPartialTitle": "Deleted board \"{{name}}\" with partial media cleanup", "deleteBoardQuestion": "Delete board \"{{name}}\"?", "deleteBoardStarredKept": "{{images}} and {{videos}} starred kept in Uncategorized.", - "deleteBoardStarredNotice_one": "Starred media is protected: {{count}} starred item on this board will move to Uncategorized instead of being deleted.", - "deleteBoardStarredNotice_other": "Starred media is protected: {{count}} starred items on this board will move to Uncategorized instead of being deleted.", + "deleteBoardStarredNotice_one": "Starred media is protected: Delete Board and Media keeps {{count}} starred item on this board and moves it to Uncategorized.", + "deleteBoardStarredNotice_other": "Starred media is protected: Delete Board and Media keeps {{count}} starred items on this board and moves them to Uncategorized.", + "deleteBoardStarredNoticeUnknown": "Starred media is protected, but the starred items on this board could not be counted. They are kept either way.", "deleteBoardSuccessTitle": "Deleted board \"{{name}}\"", "deleteConfirmLabel": "Delete", "deleteConfirmSkip": "Don't ask me again", diff --git a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.browser.test.tsx b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.browser.test.tsx index 3b11d24f0fd..ade0bb83a77 100644 --- a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.browser.test.tsx +++ b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.browser.test.tsx @@ -15,13 +15,15 @@ import { GalleryBoardMenu } from './GalleryBoardMenu'; import { GalleryUiProvider, type GalleryUiAdapter } from './GalleryUiContext'; import { GalleryWidgetContext } from './GalleryWidgetContext'; -const mocks = vi.hoisted(() => ({ starredCount: 0 })); +const mocks = vi.hoisted(() => ({ starredCount: 0, starredCountFails: false })); vi.mock('@features/gallery/data/queries', async (importOriginal) => ({ ...(await importOriginal>()), galleryBoardStarredCountOptions: (boardId: string) => ({ - queryFn: () => Promise.resolve(mocks.starredCount), + queryFn: () => + mocks.starredCountFails ? Promise.reject(new Error('count unavailable')) : Promise.resolve(mocks.starredCount), queryKey: ['board-starred-count', boardId], + retry: false, }), })); @@ -40,6 +42,7 @@ vi.mock('react-i18next', () => ({ 'widgets.gallery.deleteBoardOnly': 'Delete Board Only', 'widgets.gallery.deleteBoardQuestion': `Delete board "${String(values?.name)}"?`, 'widgets.gallery.deleteBoardStarredNotice': `${String(values?.count)} starred kept`, + 'widgets.gallery.deleteBoardStarredNoticeUnknown': 'starred count unknown', 'widgets.gallery.downloadBoardWithOmission': `Download Board (${String(values?.count)} video omitted)`, 'widgets.gallery.imageCount': `${String(values?.count)} images`, 'widgets.gallery.videoCount': `${String(values?.count)} videos`, @@ -136,6 +139,7 @@ const openDeleteDialog = async () => { beforeEach(async () => { mocks.starredCount = 0; + mocks.starredCountFails = false; host = document.createElement('div'); document.body.append(host); root = createRoot(host); @@ -283,6 +287,16 @@ it('says how many starred items stay when starred media is protected', async () }); }); +it('says the starred count is unknown when it cannot be read', async () => { + mocks.starredCountFails = true; + await renderMenu(context, true); + await openDeleteDialog(); + + await vi.waitFor(() => { + expect(document.body.textContent).toContain('starred count unknown'); + }); +}); + it('adds nothing to the dialog when starred media is not protected', async () => { mocks.starredCount = 3; await renderMenu(context, false); diff --git a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.tsx b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.tsx index b5bd4819777..2cd320edb37 100644 --- a/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.tsx +++ b/invokeai/frontend/webv2/src/features/gallery/ui/GalleryBoardMenu.tsx @@ -253,18 +253,21 @@ export const GalleryBoardMenu = ({ ); }; -/** Says how many starred items a "Delete Board and Media" keeps; silent until the count is known and non-zero. */ +/** + * Says how many starred items a "Delete Board and Media" keeps. The status region stays mounted so the notice is + * announced when it arrives; it is empty while the count loads or when there is nothing to keep. + */ const BoardStarredNotice = ({ boardId }: { boardId: string }) => { const { t } = useTranslation(); - const { data: starredCount } = useQuery(galleryBoardStarredCountOptions(boardId)); - - if (!starredCount) { - return null; - } + const { data: starredCount, isError } = useQuery(galleryBoardStarredCountOptions(boardId)); return ( - {t('widgets.gallery.deleteBoardStarredNotice', { count: starredCount })} + {isError + ? t('widgets.gallery.deleteBoardStarredNoticeUnknown') + : starredCount + ? t('widgets.gallery.deleteBoardStarredNotice', { count: starredCount }) + : null} ); }; diff --git a/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.browser.test.tsx b/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.browser.test.tsx index 6314481ff7a..9532d83c73d 100644 --- a/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.browser.test.tsx +++ b/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.browser.test.tsx @@ -853,7 +853,7 @@ describe('mixed item mutation outcomes', () => { await act(async () => { await executeDeletion?.(); }); - expect(mocks.itemDelete).toHaveBeenCalledWith(refs, expect.any(AbortSignal)); + expect(mocks.itemDelete).toHaveBeenCalledWith(refs, expect.any(AbortSignal), { deleteStarred: true }); }); it('uses the localized Uncategorized label in move notifications', async () => { @@ -1047,7 +1047,7 @@ describe('starred media protection', () => { await getItemActions().deleteItems([plain, starred, starredVideo]); }); - expect(mocks.requestDeletionConfirmation).toHaveBeenCalledWith([plain], expect.any(Function), undefined); + expect(mocks.requestDeletionConfirmation).toHaveBeenCalledWith([plain], expect.any(Function), undefined, []); expect(mocks.itemDelete).toHaveBeenCalledWith([plain], expect.any(AbortSignal), { deleteStarred: false }); expect(mocks.galleryRemoveItems).toHaveBeenCalledWith(['image:plain.png']); expect(mocks.notificationsAdd).toHaveBeenCalledWith({ @@ -1073,19 +1073,26 @@ describe('starred media protection', () => { }); }); - it('stays silent about protection when deletion is not confirmed', async () => { + it('without confirmation, speaks up only when protection left nothing to delete', async () => { mocks.itemDelete.mockResolvedValue({ affectedBoardIds: ['none'], failed: [], succeeded: [plain] }); await act(async () => { await getItemActions().deleteItems([starred]); }); + + expect(mocks.itemDelete).not.toHaveBeenCalled(); + expect(mocks.notificationsAdd).toHaveBeenCalledExactlyOnceWith({ + kind: 'info', + title: 'widgets.gallery.itemActions.delete.starredOnly:1:undefined', + }); + + mocks.notificationsAdd.mockClear(); await act(async () => { await getItemActions().deleteItems([plain, starred]); }); expect(mocks.itemDelete).toHaveBeenCalledOnce(); - expect(mocks.notificationsAdd).toHaveBeenCalledOnce(); - expect(mocks.notificationsAdd).toHaveBeenCalledWith({ + expect(mocks.notificationsAdd).toHaveBeenCalledExactlyOnceWith({ kind: 'success', message: undefined, title: 'widgets.gallery.itemActions.delete.success', @@ -1245,6 +1252,36 @@ describe('total transport failure rollback', () => { ); }); + it('keeps the gallery references of an item the backend protected while the deleted one stays removed', async () => { + const deletedItem = { kind: 'image' as const, name: 'deleted.png' }; + const protectedItem = { kind: 'image' as const, name: 'starred.png' }; + const before = makeMockProject('project-1', { recentImages: [recentImageFixture] }); + const afterRemoval = makeMockProject('project-1', { recentImages: [] }); + + mocks.getSnapshot.mockReturnValueOnce({ activeProject: before, projects: [before] }).mockReturnValue({ + activeProject: afterRemoval, + projects: [afterRemoval], + }); + mocks.itemDelete.mockResolvedValue({ + affectedBoardIds: ['none'], + failed: [protectedItem], + starredSkipped: [protectedItem], + succeeded: [deletedItem], + }); + + await act(async () => { + await getItemActions().deleteItems([deletedItem, protectedItem]); + }); + + expect(mocks.galleryWidgetsPatchValues).toHaveBeenCalledWith( + 'gallery', + { recentImages: [recentImageFixture] }, + 'project-1', + 'system' + ); + expect(mocks.galleryRemoveItems).toHaveBeenLastCalledWith(['image:deleted.png']); + }); + it('does not clobber a gallery widget field something else changed before the rollback runs', async () => { const refs = [{ kind: 'image' as const, name: 'gone.png' }]; const before = makeMockProject('project-1', { recentImages: [recentImageFixture] }); diff --git a/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.ts b/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.ts index abb8c627137..c42c33c4a62 100644 --- a/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.ts +++ b/invokeai/frontend/webv2/src/workbench/image-actions/useImageActions.ts @@ -322,7 +322,7 @@ export const useImageActions = ({ new Error( t(`widgets.gallery.itemActions.${action}.${result.succeeded.length > 0 ? 'partial' : 'failure'}`, { board: boardId ? getBoardName(boardId) : undefined, - count: requestedCount + withheldStarredCount - skippedStarredCount, + count: requestedCount - keptStarredCount, failed: failedCount, succeeded: result.succeeded.length, }) @@ -331,25 +331,20 @@ export const useImageActions = ({ return; } - // Protection is a deliberate choice, so it is reported only where deletion is confirmed; otherwise it stays - // silent and the starred items simply remain. - if (skippedStarredCount > 0) { - if (confirmImageDeletion) { - notifications.add({ - kind: 'info', - title: t( - result.succeeded.length > 0 - ? 'widgets.gallery.itemActions.delete.starredKept' - : 'widgets.gallery.itemActions.delete.starredOnly', - { count: skippedStarredCount, succeeded: result.succeeded.length } - ), - }); - return; - } - - if (result.succeeded.length === 0) { - return; - } + // Protection is a deliberate choice, so what it kept is reported where deletion is confirmed. Without + // confirmation the success message stands alone, unless nothing was deleted: a key press that does nothing + // would look broken. + if (skippedStarredCount > 0 && (confirmImageDeletion || result.succeeded.length === 0)) { + notifications.add({ + kind: 'info', + title: t( + result.succeeded.length > 0 + ? 'widgets.gallery.itemActions.delete.starredKept' + : 'widgets.gallery.itemActions.delete.starredOnly', + { count: skippedStarredCount, succeeded: result.succeeded.length } + ), + }); + return; } recordSuccess( @@ -478,8 +473,6 @@ export const useImageActions = ({ } if (result.succeeded.length === 0) { - // Nothing went away (every item was refused or kept), so the widget values cleared up front come back too. - restoreGalleryItemRemoval(galleryWidgetSnapshot); return; } @@ -598,7 +591,7 @@ export const useImageActions = ({ const withheldStarredCount = items.length - deletable.length; if (deletable.length === 0) { - if (withheldStarredCount > 0 && confirmImageDeletion) { + if (withheldStarredCount > 0) { notifications.add({ kind: 'info', title: t('widgets.gallery.itemActions.delete.starredOnly', { count: withheldStarredCount }), diff --git a/tests/app/services/image_records/test_image_records.py b/tests/app/services/image_records/test_image_records.py index 625b50e7874..8247c2dee25 100644 --- a/tests/app/services/image_records/test_image_records.py +++ b/tests/app/services/image_records/test_image_records.py @@ -393,7 +393,9 @@ def test_names_spanning_several_statements( assert store.get(survivor).is_intermediate is False assert _intermediates(database) == [] - @pytest.mark.parametrize("operation", ["delete_intermediates_by_names", "delete_many", "get_subfolders"]) + @pytest.mark.parametrize( + "operation", ["delete_intermediates_by_names", "delete_many", "delete_many_keeping_starred", "get_subfolders"] + ) def test_every_name_is_covered_and_no_statement_binds_more_than_sqlite_takes( self, database: Database, store: ImageRecordStorage, operation: str ) -> None: diff --git a/tests/app/services/images/test_images_default.py b/tests/app/services/images/test_images_default.py index 7fd545d20b0..8c68d3e22fb 100644 --- a/tests/app/services/images/test_images_default.py +++ b/tests/app/services/images/test_images_default.py @@ -1506,10 +1506,11 @@ def test_starred_image_is_preserved_when_protected(self, image_service: ImageSer def test_unstarred_image_is_deleted_when_protected(self, image_service: ImageService): invoker = image_service._ImageService__invoker # type: ignore invoker.services.image_records.get.return_value = _make_record(starred=False) + invoker.services.image_records.delete_many_keeping_starred.return_value = [] assert image_service.delete("normal.png", delete_starred=False) is True - invoker.services.image_records.delete.assert_called_once_with("normal.png") + invoker.services.image_records.delete_many_keeping_starred.assert_called_once_with(["normal.png"]) def test_board_delete_reports_protected_starred_images(self, image_service: ImageService): invoker = image_service._ImageService__invoker # type: ignore @@ -1521,13 +1522,14 @@ def test_board_delete_reports_protected_starred_images(self, image_service: Imag _make_record(image_name="starred.png", starred=True), _make_record(image_name="normal.png", starred=False), ] + invoker.services.image_records.delete_many_keeping_starred.return_value = [] deleted, failed, starred_skipped = image_service.delete_images_on_board("board-1", delete_starred=False) assert deleted == ["normal.png"] assert failed == [] assert starred_skipped == ["starred.png"] - invoker.services.image_records.delete_many.assert_called_once_with(["normal.png"]) + invoker.services.image_records.delete_many_keeping_starred.assert_called_once_with(["normal.png"]) invoker.services.image_files.stage_delete.assert_called_once_with("normal.png", image_subfolder="") def test_board_delete_removes_starred_images_by_default(self, image_service: ImageService): @@ -1572,6 +1574,44 @@ def test_unprotected_batch_still_deletes_starred_images(self, wired) -> None: assert svc.delete_images_by_names(["star.png"]) == (["star.png"], [], []) assert not storage.get_path("star.png").exists() + def test_an_image_starred_during_a_protected_batch_keeps_its_record_and_files(self, wired, monkeypatch) -> None: + svc, records, storage = wired + self._seed(wired, "plain.png", starred=False) + self._seed(wired, "late.png", starred=False) + real_delete = records.delete_many_keeping_starred + + def starred_while_files_are_staged(image_names: list[str]) -> list[str]: + records.update("late.png", ImageRecordChanges(starred=True)) + return real_delete(image_names) + + monkeypatch.setattr(records, "delete_many_keeping_starred", starred_while_files_are_staged) + + result = svc.delete_images_by_names(["plain.png", "late.png"], delete_starred=False) + + assert result == (["plain.png"], [], ["late.png"]) + assert records.get("late.png").starred is True + assert storage.get_path("late.png").exists() + assert storage.get_path("late.png", thumbnail=True).exists() + assert not storage.get_path("plain.png").exists() + assert _staging_dirs(storage) == [] + + def test_an_image_starred_during_a_protected_single_delete_is_kept(self, wired, monkeypatch) -> None: + svc, records, storage = wired + self._seed(wired, "late.png", starred=False) + real_delete = records.delete_many_keeping_starred + + def starred_before_the_delete(image_names: list[str]) -> list[str]: + records.update("late.png", ImageRecordChanges(starred=True)) + return real_delete(image_names) + + monkeypatch.setattr(records, "delete_many_keeping_starred", starred_before_the_delete) + + assert svc.delete("late.png", delete_starred=False) is False + + assert records.get("late.png").starred is True + assert storage.get_path("late.png").exists() + assert _staging_dirs(storage) == [] + def test_protected_single_delete_leaves_a_starred_image_and_no_staging_behind(self, wired) -> None: svc, records, storage = wired self._seed(wired, "star.png", starred=True) diff --git a/tests/app/services/shared/test_bulk_media_delete.py b/tests/app/services/shared/test_bulk_media_delete.py index aabe8a56e95..461c5b09d42 100644 --- a/tests/app/services/shared/test_bulk_media_delete.py +++ b/tests/app/services/shared/test_bulk_media_delete.py @@ -16,6 +16,7 @@ def _adapter( *, stage_failures: frozenset[str] = frozenset(), delete_records: MagicMock | None = None, + delete_unstarred_records: MagicMock | None = None, ) -> tuple[StagedMediaDeleteAdapter[_Record], MagicMock, MagicMock, MagicMock]: rollback = MagicMock() commit = MagicMock() @@ -32,6 +33,7 @@ def stage(name: str, record: _Record) -> object: is_starred=lambda record: record.starred, stage=stage, delete_records=delete_records or MagicMock(), + delete_unstarred_records=delete_unstarred_records or MagicMock(return_value=[]), rollback=rollback, commit=commit, notify_deleted=notify, @@ -50,25 +52,25 @@ def test_starred_names_are_deleted_unless_protection_is_requested() -> None: def test_protection_skips_starred_names_without_staging_or_deleting_them() -> None: - delete_records = MagicMock() - adapter, rollback, commit, notify = _adapter(dict(RECORDS), delete_records=delete_records) + delete_unstarred_records = MagicMock(return_value=[]) + adapter, rollback, commit, notify = _adapter(dict(RECORDS), delete_unstarred_records=delete_unstarred_records) result = delete_media_by_names(["plain.png", "star.png"], adapter, delete_starred=False) assert result == (["plain.png"], [], ["star.png"]) - delete_records.assert_called_once_with(["plain.png"]) + delete_unstarred_records.assert_called_once_with(["plain.png"]) commit.assert_called_once_with("token:plain.png") notify.assert_called_once_with("plain.png") rollback.assert_not_called() def test_protection_with_only_starred_names_deletes_no_records() -> None: - delete_records = MagicMock() - adapter, _, commit, notify = _adapter(dict(RECORDS), delete_records=delete_records) + delete_unstarred_records = MagicMock(return_value=[]) + adapter, _, commit, notify = _adapter(dict(RECORDS), delete_unstarred_records=delete_unstarred_records) assert delete_media_by_names(["star.png"], adapter, delete_starred=False) == ([], [], ["star.png"]) - delete_records.assert_called_once_with([]) + delete_unstarred_records.assert_called_once_with([]) commit.assert_not_called() notify.assert_not_called() @@ -82,8 +84,8 @@ def test_a_failed_stage_is_reported_failed_not_starred_and_protection_still_appl def test_a_record_delete_failure_restores_only_the_staged_items() -> None: - delete_records = MagicMock(side_effect=RuntimeError("database is locked")) - adapter, rollback, commit, notify = _adapter(dict(RECORDS), delete_records=delete_records) + delete_unstarred_records = MagicMock(side_effect=RuntimeError("database is locked")) + adapter, rollback, commit, notify = _adapter(dict(RECORDS), delete_unstarred_records=delete_unstarred_records) with pytest.raises(RuntimeError): delete_media_by_names(["plain.png", "star.png"], adapter, delete_starred=False) @@ -91,3 +93,15 @@ def test_a_record_delete_failure_restores_only_the_staged_items() -> None: rollback.assert_called_once_with("token:plain.png") commit.assert_not_called() notify.assert_not_called() + + +def test_a_name_starred_after_it_was_read_is_kept_and_its_files_are_restored() -> None: + delete_unstarred_records = MagicMock(return_value=["plain.png"]) + adapter, rollback, commit, notify = _adapter(dict(RECORDS), delete_unstarred_records=delete_unstarred_records) + + result = delete_media_by_names(["plain.png", "other.png"], adapter, delete_starred=False) + + assert result == (["other.png"], [], ["plain.png"]) + rollback.assert_called_once_with("token:plain.png") + commit.assert_called_once_with("token:other.png") + notify.assert_called_once_with("other.png") diff --git a/tests/app/services/video_records/test_video_records.py b/tests/app/services/video_records/test_video_records.py index f0b4565e2b6..925fade2ffc 100644 --- a/tests/app/services/video_records/test_video_records.py +++ b/tests/app/services/video_records/test_video_records.py @@ -652,7 +652,9 @@ def test_a_size_backfill_fills_only_sizes_not_known_yet(store: VideoRecordStorag assert [store.get(name).file_size_bytes for name in ("unknown.mp4", "measured.mp4")] == [7, 100] -@pytest.mark.parametrize("operation", ["delete_intermediates_by_names", "delete_many", "get_subfolders"]) +@pytest.mark.parametrize( + "operation", ["delete_intermediates_by_names", "delete_many", "delete_many_keeping_starred", "get_subfolders"] +) def test_every_name_is_covered_and_no_statement_binds_more_than_sqlite_takes( database: Database, store: VideoRecordStorage, operation: str ) -> None: diff --git a/tests/app/services/videos/test_videos_default.py b/tests/app/services/videos/test_videos_default.py index 4a57fa93b4c..1031b4f1031 100644 --- a/tests/app/services/videos/test_videos_default.py +++ b/tests/app/services/videos/test_videos_default.py @@ -215,10 +215,21 @@ def test_starred_video_is_preserved_when_protected(self, video_service: VideoSer def test_unstarred_video_is_deleted_when_protected(self, video_service: VideoService): invoker = video_service._VideoService__invoker # type: ignore[attr-defined] invoker.services.video_records.get.return_value = _make_record(starred=False) + invoker.services.video_records.delete_many_keeping_starred.return_value = [] assert video_service.delete("normal.mp4", delete_starred=False) is True - invoker.services.video_records.delete.assert_called_once_with("normal.mp4") + invoker.services.video_records.delete_many_keeping_starred.assert_called_once_with(["normal.mp4"]) + + def test_video_starred_after_the_check_is_kept_and_its_files_restored(self, video_service: VideoService): + invoker = video_service._VideoService__invoker # type: ignore[attr-defined] + invoker.services.video_records.get.return_value = _make_record(starred=False) + invoker.services.video_records.delete_many_keeping_starred.return_value = ["late.mp4"] + + assert video_service.delete("late.mp4", delete_starred=False) is False + + invoker.services.video_files.rollback_delete.assert_called_once() + invoker.services.video_files.commit_delete.assert_not_called() def test_board_delete_reports_protected_starred_videos(self, video_service: VideoService): invoker = video_service._VideoService__invoker # type: ignore[attr-defined] @@ -230,13 +241,14 @@ def test_board_delete_reports_protected_starred_videos(self, video_service: Vide _make_record(video_name="starred.mp4", starred=True), _make_record(video_name="normal.mp4", starred=False), ] + invoker.services.video_records.delete_many_keeping_starred.return_value = [] deleted, failed, starred_skipped = video_service.delete_videos_on_board("board-1", delete_starred=False) assert deleted == ["normal.mp4"] assert failed == [] assert starred_skipped == ["starred.mp4"] - invoker.services.video_records.delete_many.assert_called_once_with(["normal.mp4"]) + invoker.services.video_records.delete_many_keeping_starred.assert_called_once_with(["normal.mp4"]) invoker.services.video_files.stage_delete.assert_called_once_with("normal.mp4", video_subfolder="") def test_board_delete_removes_starred_videos_by_default(self, video_service: VideoService):