feat(web): drop "Remove from library" from the track kebab
test-web / test (push) Successful in 32s

No single-click destructive action belongs in the kebab. Removing the item
orphaned its whole path (RemoveTrackPopover was its only caller, and the
admin/tracks API client was the popover's only caller), so per the repo's
no-dead-code convention the chain is fully removed: the menu item + its
admin/isAdmin plumbing in TrackMenu, RemoveTrackPopover(.svelte/.test),
src/lib/api/admin/tracks(.ts/.test), and the now-needless transitive mocks
in the CompactTrackCard / PlaylistTrackRow / playlist specs.

The kebab is now an 8-item, admin-agnostic menu. The DELETE /api/admin/tracks
server endpoint is untouched — a future safer admin surface can rebind it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-06-14 22:17:35 -04:00
parent 9a166f9032
commit f7278f2417
9 changed files with 6 additions and 357 deletions
-64
View File
@@ -1,64 +0,0 @@
import { afterEach, describe, expect, test, vi } from 'vitest';
import { removeTrack } from './tracks';
afterEach(() => {
vi.unstubAllGlobals();
});
function stubFetch(status: number, body: unknown, init: Partial<Response> = {}) {
const res = new Response(
body === null ? null : JSON.stringify(body),
{ status, headers: { 'Content-Type': 'application/json' }, ...init }
);
const spy = vi.fn().mockResolvedValue(res);
vi.stubGlobal('fetch', spy);
return spy;
}
describe('removeTrack', () => {
test('DELETEs /api/admin/tracks/:id with no query string when unmonitor unset', async () => {
const spy = stubFetch(200, {
deleted_track_id: 't-1',
deleted_album_id: 'al-1'
});
const r = await removeTrack('t-1');
expect(r.deleted_track_id).toBe('t-1');
expect(r.deleted_album_id).toBe('al-1');
expect(r.deleted_artist_id).toBeUndefined();
expect(r.lidarr_unmonitor_failed).toBeUndefined();
const call = spy.mock.calls[0];
expect(call[0]).toBe('/api/admin/tracks/t-1');
expect((call[1] as RequestInit).method).toBe('DELETE');
});
test('appends ?unmonitor=true when option set', async () => {
const spy = stubFetch(200, {
deleted_track_id: 't-1',
lidarr_unmonitor_failed: true
});
const r = await removeTrack('t-1', { unmonitor: true });
expect(r.lidarr_unmonitor_failed).toBe(true);
const call = spy.mock.calls[0];
expect(call[0]).toBe('/api/admin/tracks/t-1?unmonitor=true');
});
test('omits the query string when unmonitor: false', async () => {
const spy = stubFetch(200, { deleted_track_id: 't-1' });
await removeTrack('t-1', { unmonitor: false });
const call = spy.mock.calls[0];
expect(call[0]).toBe('/api/admin/tracks/t-1');
});
test('surfaces not_found as ApiError', async () => {
stubFetch(404, { error: 'not_found' });
await expect(removeTrack('t-1')).rejects.toMatchObject({
code: 'not_found'
});
});
});
-42
View File
@@ -1,42 +0,0 @@
import { apiFetch } from '$lib/api/client';
export type RemoveTrackResult = {
deleted_track_id: string;
deleted_album_id?: string;
deleted_artist_id?: string;
/** Only present (always `true` when set) when the operator requested
* unmonitor=true AND the Lidarr unmonitor call failed. The destructive
* file + DB delete already succeeded — surface a follow-up toast so
* the operator knows to manually unmonitor in Lidarr. */
lidarr_unmonitor_failed?: true;
};
export type RemoveTrackOptions = {
/** When true, after the file + DB delete the server calls
* Lidarr.UnmonitorTrack so Lidarr won't search for a replacement.
* When false / omitted, no Lidarr call — Lidarr's monitoring will
* re-import on next scan, which is the operator's "find a replacement"
* path. */
unmonitor?: boolean;
};
/**
* DELETE /api/admin/tracks/{id} — admin-only. Deletes the file from
* disk and the DB row, cascading album/artist tidy-up. Optionally
* unmonitors the track in Lidarr.
*
* On success returns the deleted track id plus optional album/artist
* ids when their parent row was tidied up by the server-side cascade.
* Caller is responsible for invalidating any TanStack Query keys the
* deleted entities backed.
*/
export async function removeTrack(
id: string,
options: RemoveTrackOptions = {}
): Promise<RemoveTrackResult> {
const params = new URLSearchParams();
if (options.unmonitor) params.set('unmonitor', 'true');
const qs = params.toString();
const url = `/api/admin/tracks/${encodeURIComponent(id)}${qs ? '?' + qs : ''}`;
return (await apiFetch(url, { method: 'DELETE' })) as RemoveTrackResult;
}
@@ -10,10 +10,6 @@ vi.mock('$lib/api/likes', () => emptyLikesMock());
vi.mock('$lib/api/quarantine', () => emptyQuarantineMock());
vi.mock('$lib/api/admin/tracks', () => ({
removeTrack: vi.fn().mockResolvedValue({ deleted_track_id: 't1' })
}));
vi.mock('$lib/api/playlists', () => emptyPlaylistsMock());
vi.mock('$lib/auth/store.svelte', () => ({
@@ -21,10 +21,6 @@ vi.mock('$lib/api/likes', () => emptyLikesMock());
vi.mock('$lib/api/quarantine', () => emptyQuarantineMock());
vi.mock('$lib/api/admin/tracks', () => ({
removeTrack: vi.fn().mockResolvedValue({ deleted_track_id: 't-1' })
}));
vi.mock('$lib/player/store.svelte', () => ({
playNext: vi.fn(),
enqueueTrack: vi.fn(),
@@ -1,103 +0,0 @@
<script lang="ts">
import { Trash2 } from 'lucide-svelte';
import { useQueryClient } from '@tanstack/svelte-query';
import { removeTrack } from '$lib/api/admin/tracks';
import { errMessage } from '$lib/api/errors';
import { qk } from '$lib/api/queries';
import type { TrackRef } from '$lib/api/types';
let {
track,
onClose
}: {
track: TrackRef;
onClose: () => void;
} = $props();
const queryClient = useQueryClient();
let unmonitor = $state(false);
let busy = $state(false);
let error = $state<string | null>(null);
async function confirm() {
busy = true;
error = null;
try {
const result = await removeTrack(track.id, { unmonitor });
// Invalidate caches for any vanished entity. The album/artist that
// contained the track is always touched; if the parent rows were
// tidied up by the server cascade, evict those too. Home payload
// surfaces albums/artists on the landing page so it always
// refreshes.
await queryClient.invalidateQueries({ queryKey: qk.album(track.album_id) });
await queryClient.invalidateQueries({ queryKey: qk.artist(track.artist_id) });
await queryClient.invalidateQueries({ queryKey: qk.home() });
if (result.deleted_album_id) {
await queryClient.invalidateQueries({ queryKey: qk.album(result.deleted_album_id) });
}
if (result.deleted_artist_id) {
await queryClient.invalidateQueries({ queryKey: qk.artist(result.deleted_artist_id) });
}
// Note: result.lidarr_unmonitor_failed is true when unmonitor was
// requested AND Lidarr couldn't be reached. The destructive part
// already succeeded; we still close the popover. A future toast
// surface (if any) can pick up the flag.
onClose();
} catch (e: unknown) {
error = errMessage(e);
} finally {
busy = false;
}
}
</script>
<div
role="dialog"
tabindex="-1"
aria-modal="false"
aria-labelledby="remove-track-popover-title"
class="absolute right-0 z-30 mt-1 w-72 rounded-lg border border-border bg-surface p-3 shadow-xl"
onclick={(e) => e.stopPropagation()}
onkeydown={(e) => { e.stopPropagation(); if (e.key === 'Escape') onClose(); }}
>
<h4 id="remove-track-popover-title" class="text-sm font-medium text-text-primary">
Remove "{track.title}" from your library?
</h4>
<p class="mt-1 text-xs text-text-muted">
This deletes the file from disk.
</p>
<label class="mt-3 flex items-start gap-2 text-sm text-text-primary">
<input
type="checkbox"
bind:checked={unmonitor}
class="mt-0.5 rounded border-border"
/>
<span>Also stop Lidarr from finding a replacement</span>
</label>
{#if error}
<p class="mt-2 text-xs text-action-destructive">{error}</p>
{/if}
<div class="mt-3 flex justify-end gap-2">
<button
type="button"
class="rounded-md border border-border px-2.5 py-1 text-sm text-text-secondary hover:text-text-primary disabled:opacity-50"
onclick={onClose}
disabled={busy}
>
Cancel
</button>
<button
type="button"
onclick={confirm}
disabled={busy}
class="inline-flex items-center gap-1 rounded-md border border-border bg-surface px-2.5 py-1 text-sm text-action-destructive hover:bg-surface-hover disabled:opacity-50"
>
<Trash2 size={14} strokeWidth={1} />
{busy ? 'Removing…' : 'Remove'}
</button>
</div>
</div>
@@ -1,97 +0,0 @@
import { afterEach, describe, expect, test, vi } from 'vitest';
import { render, screen, fireEvent, waitFor } from '@testing-library/svelte';
import { makeTrack } from '$test-utils/fixtures/track';
const invalidateMock = vi.fn();
vi.mock('@tanstack/svelte-query', async (orig) => {
const actual = (await orig()) as Record<string, unknown>;
return {
...actual,
useQueryClient: () => ({ invalidateQueries: invalidateMock })
};
});
vi.mock('$lib/api/admin/tracks', () => ({
removeTrack: vi.fn()
}));
import RemoveTrackPopover from './RemoveTrackPopover.svelte';
import { removeTrack } from '$lib/api/admin/tracks';
const track = makeTrack({ title: 'Roygbiv' });
afterEach(() => vi.clearAllMocks());
describe('RemoveTrackPopover', () => {
test('renders the title and subtext', () => {
render(RemoveTrackPopover, { props: { track, onClose: vi.fn() } });
expect(screen.getByText(/remove "roygbiv" from your library/i)).toBeInTheDocument();
expect(screen.getByText(/this deletes the file from disk/i)).toBeInTheDocument();
});
test('renders the Lidarr unmonitor checkbox', () => {
render(RemoveTrackPopover, { props: { track, onClose: vi.fn() } });
const cb = screen.getByRole('checkbox', { name: /also stop lidarr/i });
expect(cb).toBeInTheDocument();
expect((cb as HTMLInputElement).checked).toBe(false);
});
test('Cancel button calls onClose without firing removeTrack', async () => {
const onClose = vi.fn();
render(RemoveTrackPopover, { props: { track, onClose } });
await fireEvent.click(screen.getByRole('button', { name: /cancel/i }));
expect(removeTrack).not.toHaveBeenCalled();
expect(onClose).toHaveBeenCalled();
});
test('Remove without checkbox calls removeTrack with unmonitor=false', async () => {
(removeTrack as ReturnType<typeof vi.fn>).mockResolvedValue({ deleted_track_id: 't1' });
const onClose = vi.fn();
render(RemoveTrackPopover, { props: { track, onClose } });
await fireEvent.click(screen.getByRole('button', { name: /^remove$/i }));
// confirm() chains 3-5 awaited invalidateQueries calls before onClose,
// each a separate microtask. waitFor polls until the side effects land.
await waitFor(() => expect(onClose).toHaveBeenCalled());
expect(removeTrack).toHaveBeenCalledWith('t1', { unmonitor: false });
});
test('Remove with checkbox checked calls removeTrack with unmonitor=true', async () => {
(removeTrack as ReturnType<typeof vi.fn>).mockResolvedValue({ deleted_track_id: 't1' });
const onClose = vi.fn();
render(RemoveTrackPopover, { props: { track, onClose } });
await fireEvent.click(screen.getByRole('checkbox', { name: /also stop lidarr/i }));
await fireEvent.click(screen.getByRole('button', { name: /^remove$/i }));
await waitFor(() => expect(onClose).toHaveBeenCalled());
expect(removeTrack).toHaveBeenCalledWith('t1', { unmonitor: true });
});
test('failed remove surfaces error via copyForCode and does not close', async () => {
(removeTrack as ReturnType<typeof vi.fn>).mockRejectedValue({ code: 'not_found' });
const onClose = vi.fn();
render(RemoveTrackPopover, { props: { track, onClose } });
await fireEvent.click(screen.getByRole('button', { name: /^remove$/i }));
await waitFor(() => expect(removeTrack).toHaveBeenCalled());
// The catch path runs synchronously after the rejected await; one more
// microtask boundary is enough.
await Promise.resolve();
expect(onClose).not.toHaveBeenCalled();
});
test('success invalidates album, artist and home queries', async () => {
(removeTrack as ReturnType<typeof vi.fn>).mockResolvedValue({
deleted_track_id: 't1',
deleted_album_id: 'a1',
deleted_artist_id: 'ar1'
});
render(RemoveTrackPopover, { props: { track, onClose: vi.fn() } });
await fireEvent.click(screen.getByRole('button', { name: /^remove$/i }));
// Wait for all three baseline invalidations (album, artist, home) plus
// the two cascade ones (deleted_album_id, deleted_artist_id) to land.
await waitFor(() => expect(invalidateMock.mock.calls.length).toBeGreaterThanOrEqual(5));
const keys = invalidateMock.mock.calls.map((c) => c[0].queryKey);
const flat = keys.map((k) => Array.isArray(k) ? k.join('|') : String(k));
expect(flat.some((k) => k.startsWith('album|a1'))).toBe(true);
expect(flat.some((k) => k.startsWith('artist|ar1'))).toBe(true);
expect(flat.some((k) => k === 'home')).toBe(true);
});
});
+4 -29
View File
@@ -10,17 +10,14 @@
Disc3,
User,
EyeOff,
Eye,
Trash2
Eye
} from 'lucide-svelte';
import { goto } from '$app/navigation';
import { useQueryClient } from '@tanstack/svelte-query';
import FlagPopover from './FlagPopover.svelte';
import RemoveTrackPopover from './RemoveTrackPopover.svelte';
import AddToPlaylistMenu from './AddToPlaylistMenu.svelte';
import TrackMenuItem from './TrackMenuItem.svelte';
import TrackMenuDivider from './TrackMenuDivider.svelte';
import { user } from '$lib/auth/store.svelte';
import { createLikedIdsQuery, likeEntity, unlikeEntity } from '$lib/api/likes';
import { createMyQuarantineQuery, unflagTrack } from '$lib/api/quarantine';
import { qk } from '$lib/api/queries';
@@ -44,7 +41,6 @@
let menuOpen = $state(false);
let flagOpen = $state(false);
let removeOpen = $state(false);
let addOpen = $state(false);
const queryClient = useQueryClient();
@@ -63,8 +59,6 @@
return rows.some((r) => r.track_id === track.id);
});
const isAdmin = $derived(user.value?.is_admin === true);
function toggleMenu(e: MouseEvent) {
e.stopPropagation();
menuOpen = !menuOpen;
@@ -73,7 +67,6 @@
function closeAll() {
menuOpen = false;
flagOpen = false;
removeOpen = false;
addOpen = false;
}
@@ -124,13 +117,8 @@
goto(`/artists/${track.artist_id}`);
}
function onRemoveOpen() {
menuOpen = false;
removeOpen = true;
}
function onKeydown(e: KeyboardEvent) {
if (!menuOpen && !flagOpen && !removeOpen && !addOpen) return;
if (!menuOpen && !flagOpen && !addOpen) return;
if (e.key === 'Escape') {
e.preventDefault();
closeAll();
@@ -166,8 +154,8 @@
add-to-playlist / start-radio group → navigation group → hide.
"Start radio" is shown even under hideQueueActions (reseeding a
station from the current track is meaningful where play-next /
add-to-queue are not). The admin-only "Remove from library" is a
web superset Android has no surface for — kept past the divider. -->
add-to-queue are not). No single-click destructive action lives
here — track removal is intentionally kept out of the kebab. -->
{#if !hideQueueActions}
<TrackMenuItem icon={ListVideo} label="Play next" onclick={onPlayNext} />
<TrackMenuItem icon={ListMusic} label="Add to queue" onclick={onAddToQueue} />
@@ -199,15 +187,6 @@
label={hidden ? 'Unhide' : 'Hide'}
onclick={onToggleHide}
/>
{#if isAdmin}
<TrackMenuDivider />
<TrackMenuItem
icon={Trash2}
label="Remove from library"
onclick={onRemoveOpen}
danger
/>
{/if}
</div>
{/if}
@@ -215,10 +194,6 @@
<FlagPopover {track} onClose={closeAll} />
{/if}
{#if removeOpen}
<RemoveTrackPopover {track} onClose={closeAll} />
{/if}
{#if addOpen}
<AddToPlaylistMenu tracks={[track]} onClose={() => (addOpen = false)} />
{/if}
+2 -13
View File
@@ -25,10 +25,6 @@ vi.mock('$lib/api/likes', () => emptyLikesMock());
vi.mock('$lib/api/quarantine', () => emptyQuarantineMock());
vi.mock('$lib/api/admin/tracks', () => ({
removeTrack: vi.fn().mockResolvedValue({ deleted_track_id: 't1' })
}));
vi.mock('$lib/api/playlists', () => emptyPlaylistsMock());
vi.mock('$lib/player/store.svelte', () => ({
@@ -56,7 +52,7 @@ describe('TrackMenu', () => {
expect(kebab.getAttribute('aria-expanded')).toBe('true');
});
test('opening menu shows all 9 entries for an admin user', async () => {
test('opening menu shows all 8 entries', async () => {
render(TrackMenu, { props: { track } });
await fireEvent.click(screen.getByRole('button', { name: /track actions/i }));
@@ -68,15 +64,8 @@ describe('TrackMenu', () => {
expect(screen.getByRole('menuitem', { name: /go to album/i })).toBeInTheDocument();
expect(screen.getByRole('menuitem', { name: /go to artist/i })).toBeInTheDocument();
expect(screen.getByRole('menuitem', { name: /^hide$/i })).toBeInTheDocument();
expect(screen.getByRole('menuitem', { name: /remove from library/i })).toBeInTheDocument();
expect(screen.getAllByRole('menuitem')).toHaveLength(9);
});
test('"Remove from library" hidden for non-admin', async () => {
userState.current = { id: 'u2', username: 'normal', is_admin: false };
render(TrackMenu, { props: { track } });
await fireEvent.click(screen.getByRole('button', { name: /track actions/i }));
// No "Remove from library" — destructive removal is intentionally absent.
expect(screen.queryByRole('menuitem', { name: /remove from library/i })).not.toBeInTheDocument();
expect(screen.getAllByRole('menuitem')).toHaveLength(8);
});
@@ -41,7 +41,6 @@ vi.mock('$lib/api/likes', () => ({
unlikeEntity: vi.fn()
}));
vi.mock('$lib/api/quarantine', () => emptyQuarantineMock());
vi.mock('$lib/api/admin/tracks', () => ({ removeTrack: vi.fn() }));
import PlaylistDetailPage from './+page.svelte';
import { createPlaylistQuery, deletePlaylist, refreshSystem } from '$lib/api/playlists';