diff --git a/client/src/hooks/useMediaAnnotations.js b/client/src/hooks/useMediaAnnotations.js index 9484e44e20..3f923a389a 100644 --- a/client/src/hooks/useMediaAnnotations.js +++ b/client/src/hooks/useMediaAnnotations.js @@ -77,7 +77,7 @@ export function useMediaAnnotations() { return () => socket.off('media:annotation:updated', onUpdate); }, []); - const updateAnnotation = useCallback(async (key, patch) => { + const updateAnnotation = useCallback(async (key, patch, { silent = false } = {}) => { // Snapshot prior state synchronously from the ref — state updaters in // React 18 concurrent mode can be deferred or retried, so mutating an // outer `let prior` from inside `setAnnotations((prev) => { ... })` is @@ -103,8 +103,10 @@ export function useMediaAnnotations() { else next[key] = nextEntry; return next; }); + let failed = false; const res = await setMediaAnnotation(key, patch, { silent: true }).catch((err) => { - toast.error(err?.message || 'Failed to save annotation'); + failed = true; + if (!silent) toast.error(err?.message || 'Failed to save annotation'); setAnnotations((prev) => { const reverted = { ...prev }; if (prior) reverted[key] = prior; @@ -113,7 +115,10 @@ export function useMediaAnnotations() { }); return null; }); - return res?.entry ?? null; + // `entry` is legitimately `null` on a real success too (the server clears + // the entry entirely once both `starred` and `note` are empty) — `ok` is + // the only reliable success signal, entry alone can't disambiguate. + return { ok: !failed, entry: res?.entry ?? null }; }, []); // Reads prior starred state from the ref so the callback's identity stays diff --git a/client/src/pages/MediaCollectionDetail.jsx b/client/src/pages/MediaCollectionDetail.jsx index 56e5465265..f8bb6bf9f5 100644 --- a/client/src/pages/MediaCollectionDetail.jsx +++ b/client/src/pages/MediaCollectionDetail.jsx @@ -227,11 +227,21 @@ export default function MediaCollectionDetail() { const bulkStar = async (starred) => { if (selectedItems.length === 0) return; setBulkBusy(true); + let successCount = 0; + let failedCount = 0; for (const it of selectedItems) { - await updateAnnotation(it.key, { starred }); + const { ok } = await updateAnnotation(it.key, { starred }, { silent: true }); + if (ok) successCount++; else failedCount++; } setBulkBusy(false); - toast.success(`${starred ? 'Favorited' : 'Unfavorited'} ${selectedItems.length} item${selectedItems.length === 1 ? '' : 's'}`); + const verb = starred ? 'Favorited' : 'Unfavorited'; + if (failedCount === 0) { + toast.success(`${verb} ${successCount} item${successCount === 1 ? '' : 's'}`); + } else if (successCount === 0) { + toast.error(`Failed to ${starred ? 'favorite' : 'unfavorite'} items`); + } else { + toast.error(`${verb} ${successCount} item${successCount === 1 ? '' : 's'}; ${failedCount} failed`); + } }; const bulkRemove = async () => { diff --git a/client/src/pages/MediaCollectionDetail.test.jsx b/client/src/pages/MediaCollectionDetail.test.jsx index 58b6c45b05..2875abe844 100644 --- a/client/src/pages/MediaCollectionDetail.test.jsx +++ b/client/src/pages/MediaCollectionDetail.test.jsx @@ -32,11 +32,13 @@ vi.mock('../components/ui/Toast', () => ({ }), })); +const mockUpdateAnnotation = vi.fn(); + vi.mock('../hooks/useMediaAnnotations', () => ({ useMediaAnnotations: () => ({ annotations: {}, toggleStar: vi.fn(), - updateAnnotation: vi.fn(), + updateAnnotation: (...args) => mockUpdateAnnotation(...args), getCardProps: () => ({}), }), })); @@ -132,6 +134,7 @@ beforeEach(() => { mockListMediaCollections.mockResolvedValue([REAL_COLLECTION]); mockGetMediaCollection.mockResolvedValue(REAL_COLLECTION); mockPullMissingMetadata.mockResolvedValue({ attempted: 1, recovered: 1 }); + mockUpdateAnnotation.mockResolvedValue({ ok: true, entry: null }); }); // ── Unsorted view tests ─────────────────────────────────────────────────────── @@ -239,3 +242,69 @@ describe('MediaCollectionDetail — regular collection view', () => { expect(screen.queryByRole('button', { name: /pull missing prompts/i })).toBeNull(); }); }); + +// ── bulkStar (#6018): must not report false success when items fail ────────── + +describe('MediaCollectionDetail — bulkStar', () => { + 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' }, + ], + }; + + beforeEach(() => { + mockGetMediaCollection.mockResolvedValue(THREE_ITEM_COLLECTION); + }); + + async function enterSelectModeAndSelectAll(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 })); + } + + it('toasts a single success message when every item succeeds', async () => { + mockUpdateAnnotation.mockResolvedValue({ ok: true, entry: null }); + const user = userEvent.setup(); + renderReal(); + await enterSelectModeAndSelectAll(user); + + await user.click(screen.getByRole('button', { name: /^star$/i })); + + await waitFor(() => expect(mockUpdateAnnotation).toHaveBeenCalledTimes(3)); + expect(mockUpdateAnnotation.mock.calls.every(([, , opts]) => opts?.silent === true)).toBe(true); + expect(toast.success).toHaveBeenCalledWith('Favorited 3 items'); + expect(toast.error).not.toHaveBeenCalled(); + }); + + it('toasts a single failure message with no success toast when every item fails', async () => { + mockUpdateAnnotation.mockResolvedValue({ ok: false, entry: null }); + const user = userEvent.setup(); + renderReal(); + await enterSelectModeAndSelectAll(user); + + await user.click(screen.getByRole('button', { name: /^star$/i })); + + await waitFor(() => expect(mockUpdateAnnotation).toHaveBeenCalledTimes(3)); + expect(toast.error).toHaveBeenCalledWith('Failed to favorite items'); + expect(toast.success).not.toHaveBeenCalled(); + }); + + it('toasts a single consolidated message on partial failure (no contradictory success toast)', async () => { + mockUpdateAnnotation + .mockResolvedValueOnce({ ok: true, entry: null }) + .mockResolvedValueOnce({ ok: false, entry: null }) + .mockResolvedValueOnce({ ok: true, entry: null }); + const user = userEvent.setup(); + renderReal(); + await enterSelectModeAndSelectAll(user); + + await user.click(screen.getByRole('button', { name: /^star$/i })); + + await waitFor(() => expect(mockUpdateAnnotation).toHaveBeenCalledTimes(3)); + expect(toast.error).toHaveBeenCalledWith('Favorited 2 items; 1 failed'); + expect(toast.success).not.toHaveBeenCalled(); + }); +});