From 066616e196d38d4d35edaeca5dfdef0acf0a9f58 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 14 Jun 2026 21:36:26 -0400 Subject: [PATCH 1/5] fix(web): de-overlap Most Played hover controls + narrow desktop seek bar CompactTrackCard is a short one-line row but reused CardActionCluster, which corner-splits Like+Add (top) and the kebab menu (bottom). That split is right for the tall square Album/Artist cards but makes the two groups collide on the compact row's hover state. Give the compact card a single inline, vertically-centred right cluster (Like + Add + menu in one group) and widen its right padding reserve to match. In the desktop PlayerBar, the left info column was a fixed w-72 (title kept truncating) while the seek column was flex-1 (the scrubber hogged the slack on wide screens). Let the left column grow up to max-w-md while holding its 288px floor at md, and cap the seek/transport column at max-w-xl centred, so freed width flows to the title instead of stretching the bar. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../lib/components/CompactTrackCard.svelte | 38 +++++++++++++------ web/src/lib/components/PlayerBar.svelte | 13 +++++-- 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/web/src/lib/components/CompactTrackCard.svelte b/web/src/lib/components/CompactTrackCard.svelte index 2588bb7a..3452cc10 100644 --- a/web/src/lib/components/CompactTrackCard.svelte +++ b/web/src/lib/components/CompactTrackCard.svelte @@ -1,9 +1,10 @@ - - diff --git a/web/src/lib/components/RemoveTrackPopover.test.ts b/web/src/lib/components/RemoveTrackPopover.test.ts deleted file mode 100644 index 93b2505a..00000000 --- a/web/src/lib/components/RemoveTrackPopover.test.ts +++ /dev/null @@ -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; - 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).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).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).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).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); - }); -}); diff --git a/web/src/lib/components/TrackMenu.svelte b/web/src/lib/components/TrackMenu.svelte index ed0ae17c..74661f4f 100644 --- a/web/src/lib/components/TrackMenu.svelte +++ b/web/src/lib/components/TrackMenu.svelte @@ -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} @@ -199,15 +187,6 @@ label={hidden ? 'Unhide' : 'Hide'} onclick={onToggleHide} /> - {#if isAdmin} - - - {/if} {/if} @@ -215,10 +194,6 @@ {/if} - {#if removeOpen} - - {/if} - {#if addOpen} (addOpen = false)} /> {/if} diff --git a/web/src/lib/components/TrackMenu.test.ts b/web/src/lib/components/TrackMenu.test.ts index 79c58e57..31213cb8 100644 --- a/web/src/lib/components/TrackMenu.test.ts +++ b/web/src/lib/components/TrackMenu.test.ts @@ -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); }); diff --git a/web/src/routes/playlists/[id]/playlist.test.ts b/web/src/routes/playlists/[id]/playlist.test.ts index 679e3790..fd1fbfd6 100644 --- a/web/src/routes/playlists/[id]/playlist.test.ts +++ b/web/src/routes/playlists/[id]/playlist.test.ts @@ -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';