fix: stop a half-completed media collection move reporting success (#6017)

client/src/pages/MediaCollectionDetail.jsx

@@ -276,6 +276,11 @@ export default function MediaCollectionDetail() {
       });
       if (r && r !== 'dupe') { added++; placedKeys.add(it.key); }
     }
+    // A move is two writes: add to the target, then remove from here. When the
+    // second half fails the item now lives in BOTH collections — that is a
+    // half-completed move, not a success, so track those keys separately from
+    // the add failures and keep them selected for a retry.
+    const removeFailedKeys = new Set();
     if (mode === 'move' && placedKeys.size > 0) {
       if (isUnsorted) {
         // Source is synthetic — placing items in any real collection takes
@@ -286,18 +291,34 @@ export default function MediaCollectionDetail() {
         for (const it of selectedItems) {
           if (!placedKeys.has(it.key)) continue;
           const r = await removeMediaCollectionItem(collection.id, it.key, { silent: true }).catch(() => null);
-          if (r) lastOk = r;
+          if (r) lastOk = r; else removeFailedKeys.add(it.key);
         }
+        // The last successful removal carries the authoritative server state,
+        // so a partial failure still reconciles the items that did move out.
         if (lastOk) setCollection(lastOk);
       }
     }
     setBulkBusy(false);
-    exitSelectMode();
+    if (removeFailedKeys.size > 0) {
+      // Stay in select mode with only the half-moved items selected so the
+      // user can re-run the move (or remove) on exactly what failed.
+      setSelected(removeFailedKeys);
+    } else {
+      exitSelectMode();
+    }
     const verb = mode === 'move' ? (isUnsorted ? 'Filed' : 'Moved') : 'Copied';
     const note = dupes > 0 ? ` (${dupes} already there)` : '';
     const stranded = selectedItems.length - added - dupes;
-    if (stranded > 0) toast.error(`${verb} ${added} to "${targetName}"${note}; ${stranded} failed`);
-    else toast.success(`${verb} ${added} to "${targetName}"${note}`);
+    const addNote = stranded > 0 ? `; ${stranded} failed to add` : '';
+    if (removeFailedKeys.size > 0) {
+      // Deliberately says "Copied" — calling a half-completed move a "Move"
+      // is the false success this branch exists to prevent.
+      toast.error(`Copied ${added} to "${targetName}"${note}, but ${removeFailedKeys.size} could not be removed from "${collection.name}"${addNote}`);
+    } else if (stranded > 0) {
+      toast.error(`${verb} ${added} to "${targetName}"${note}; ${stranded} failed`);
+    } else {
+      toast.success(`${verb} ${added} to "${targetName}"${note}`);
+    }
   };
 
   // Remix / SendToVideo / Continue / Clean share a single implementation

client/src/pages/MediaCollectionDetail.test.jsx

@@ -10,15 +10,17 @@ const mockListImageGallery = vi.fn();
 const mockListVideoHistory = vi.fn();
 const mockListMediaCollections = vi.fn();
 const mockGetMediaCollection = vi.fn();
+const mockAddMediaCollectionItem = vi.fn();
+const mockRemoveMediaCollectionItem = vi.fn();
 
 vi.mock('../services/api', () => ({
   listImageGallery: (...args) => mockListImageGallery(...args),
   listVideoHistory: (...args) => mockListVideoHistory(...args),
   listMediaCollections: (...args) => mockListMediaCollections(...args),
   getMediaCollection: (...args) => mockGetMediaCollection(...args),
   updateMediaCollection: vi.fn(),
-  addMediaCollectionItem: vi.fn(),
-  removeMediaCollectionItem: vi.fn(),
+  addMediaCollectionItem: (...args) => mockAddMediaCollectionItem(...args),
+  removeMediaCollectionItem: (...args) => mockRemoveMediaCollectionItem(...args),
   deleteImage: vi.fn(),
   deleteVideoHistoryItem: vi.fn(),
   pullMissingMetadata: (...args) => mockPullMissingMetadata(...args),
@@ -64,8 +66,14 @@ vi.mock('../components/media/MediaPreview', () => ({
   default: () => null,
 }));
 
+// Stands in for the popover's collection rows: one clickable destination so a
+// test can drive bulkMoveOrCopy end to end.
 vi.mock('../components/media/BulkTargetPicker', () => ({
-  default: () => null,
+  default: ({ onPick }) => (
+    <button type="button" onClick={() => onPick('col-target', 'Target Collection')}>
+      pick target
+    </button>
+  ),
 }));
 
 vi.mock('../components/sharing/ShareToButton', () => ({
@@ -135,6 +143,8 @@ beforeEach(() => {
   mockGetMediaCollection.mockResolvedValue(REAL_COLLECTION);
   mockPullMissingMetadata.mockResolvedValue({ attempted: 1, recovered: 1 });
   mockUpdateAnnotation.mockResolvedValue({ ok: true, entry: null });
+  mockAddMediaCollectionItem.mockResolvedValue({ id: 'col-target', name: 'Target Collection', items: [] });
+  mockRemoveMediaCollectionItem.mockResolvedValue(REAL_COLLECTION);
 });
 
 // ── Unsorted view tests ───────────────────────────────────────────────────────
@@ -308,3 +318,97 @@ describe('MediaCollectionDetail — bulkStar', () => {
     expect(toast.success).not.toHaveBeenCalled();
   });
 });
+
+// ── bulkMoveOrCopy (#6017): a move whose removal half fails is NOT a success ──
+
+describe('MediaCollectionDetail — bulkMoveOrCopy move/remove failures', () => {
+  const KEY_A = `image:${IMAGE_A.filename}`;
+  const KEY_B = `image:${IMAGE_B.filename}`;
+  const KEY_C = `video:${VIDEO_C.id}`;
+
+  const THREE_ITEM_COLLECTION = {
+    ...REAL_COLLECTION,
+    items: [
+      { kind: 'image', ref: IMAGE_A.filename, addedAt: '2024-01-02' },
+      { kind: 'image', ref: IMAGE_B.filename, addedAt: '2024-01-01' },
+      { kind: 'video', ref: VIDEO_C.id, addedAt: '2024-01-03' },
+    ],
+  };
+  // The server state after the only successful removal in the partial test.
+  const AFTER_B_REMOVED = {
+    ...REAL_COLLECTION,
+    items: [
+      { kind: 'video', ref: VIDEO_C.id, addedAt: '2024-01-03' },
+      { kind: 'image', ref: IMAGE_A.filename, addedAt: '2024-01-02' },
+    ],
+  };
+
+  beforeEach(() => {
+    mockGetMediaCollection.mockResolvedValue(THREE_ITEM_COLLECTION);
+  });
+
+  async function selectAllAndMove(user) {
+    await waitFor(() => screen.getByRole('button', { name: /^select$/i }));
+    await user.click(screen.getByRole('button', { name: /^select$/i }));
+    await user.click(screen.getByRole('button', { name: /select all/i }));
+    await user.click(screen.getByRole('button', { name: /move…/i }));
+    await user.click(screen.getByRole('button', { name: /pick target/i }));
+  }
+
+  it('toasts a plain success and exits select mode when every removal succeeds', async () => {
+    const user = userEvent.setup();
+    renderReal();
+    await selectAllAndMove(user);
+
+    await waitFor(() => expect(mockRemoveMediaCollectionItem).toHaveBeenCalledTimes(3));
+    expect(toast.success).toHaveBeenCalledWith('Moved 3 to "Target Collection"');
+    expect(toast.error).not.toHaveBeenCalled();
+    expect(screen.queryByText(/of 3 selected/)).toBeNull();
+  });
+
+  it('reports the removal failure instead of a false success when every removal fails', async () => {
+    mockRemoveMediaCollectionItem.mockRejectedValue(new Error('boom'));
+    const user = userEvent.setup();
+    renderReal();
+    await selectAllAndMove(user);
+
+    await waitFor(() => expect(mockRemoveMediaCollectionItem).toHaveBeenCalledTimes(3));
+    expect(toast.success).not.toHaveBeenCalled();
+    expect(toast.error).toHaveBeenCalledWith(
+      'Copied 3 to "Target Collection", but 3 could not be removed from "My Collection"',
+    );
+    // Nothing left the source collection, and the half-moved items stay
+    // selected so the user can retry.
+    await waitFor(() => expect(screen.getByText('3')).toBeInTheDocument());
+    expect(screen.getByText(/of 3 selected/)).toBeInTheDocument();
+  });
+
+  it('counts add and removal failures separately and narrows the selection to the failed items', async () => {
+    // Removal order follows the rendered (newest-first) order: C, A, B.
+    mockAddMediaCollectionItem.mockImplementation((_targetId, { ref }) => (
+      ref === VIDEO_C.id
+        ? Promise.reject(new Error('add failed'))
+        : Promise.resolve({ id: 'col-target', name: 'Target Collection', items: [] })
+    ));
+    mockRemoveMediaCollectionItem.mockImplementation((_id, key) => (
+      key === KEY_A ? Promise.reject(new Error('boom')) : Promise.resolve(AFTER_B_REMOVED)
+    ));
+    const user = userEvent.setup();
+    renderReal();
+    await selectAllAndMove(user);
+
+    await waitFor(() => expect(mockRemoveMediaCollectionItem).toHaveBeenCalledTimes(2));
+    // The video never got added, so it is never removed.
+    const removedKeys = mockRemoveMediaCollectionItem.mock.calls.map(([, key]) => key);
+    expect(removedKeys).toEqual([KEY_A, KEY_B]);
+    expect(removedKeys).not.toContain(KEY_C);
+    expect(toast.success).not.toHaveBeenCalled();
+    expect(toast.error).toHaveBeenCalledWith(
+      'Copied 2 to "Target Collection", but 1 could not be removed from "My Collection"; 1 failed to add',
+    );
+    // setCollection reconciled to the last authoritative server state (B gone),
+    // and only the item whose removal failed stays selected.
+    await waitFor(() => expect(screen.getByText(/of 2 selected/)).toBeInTheDocument());
+    expect(screen.queryByText(IMAGE_B.filename)).toBeNull();
+  });
+});