diff --git a/client/src/pages/MediaCollectionDetail.jsx b/client/src/pages/MediaCollectionDetail.jsx index f8bb6bf9f5..ec2a1a3f01 100644 --- a/client/src/pages/MediaCollectionDetail.jsx +++ b/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 diff --git a/client/src/pages/MediaCollectionDetail.test.jsx b/client/src/pages/MediaCollectionDetail.test.jsx index 2875abe844..d3d6436714 100644 --- a/client/src/pages/MediaCollectionDetail.test.jsx +++ b/client/src/pages/MediaCollectionDetail.test.jsx @@ -10,6 +10,8 @@ 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), @@ -17,8 +19,8 @@ vi.mock('../services/api', () => ({ 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 }) => ( + + ), })); 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(); + }); +});