Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions client/src/hooks/useMediaAnnotations.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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
Expand Down
14 changes: 12 additions & 2 deletions client/src/pages/MediaCollectionDetail.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
71 changes: 70 additions & 1 deletion client/src/pages/MediaCollectionDetail.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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: () => ({}),
}),
}));
Expand Down Expand Up @@ -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 ───────────────────────────────────────────────────────
Expand Down Expand Up @@ -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();
});
});