From 22ac86f200d96bba3b90d97d8e95b8109b1238cb Mon Sep 17 00:00:00 2001
From: Bryan Van Deusen <bryan.vandeusen@fabledsword.com>
Date: Fri, 1 May 2026 22:23:56 -0400
Subject: [PATCH] feat(web): trim main nav; pair Discover+Requests; move chrome
 to user menu

The main nav was carrying surfaces that didn't belong on it:

- Search led to an empty page when clicked (you only ever want to land
  there from the global SearchInput typing into /search?q=...). Drop it.
- Requests is downstream of Discover (you discover, then you request).
  Lift both onto a shared horizontal tab strip; keep their URLs so
  bookmarks survive. New DiscoverTabs component used by both pages.
- Settings + Admin are operator chrome, not primary surfaces. Move them
  into the username dropdown alongside Log out, with Admin gated on
  is_admin. Users see Settings + Log out; admins see Settings + Admin
  + Log out.

Final main nav: Home / Artists / Albums / Liked / Discover / Playlists.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 web/src/lib/components/DiscoverTabs.svelte  | 36 +++++++++++++
 web/src/lib/components/DiscoverTabs.test.ts | 35 ++++++++++++
 web/src/lib/components/Shell.svelte         | 40 +++++++++-----
 web/src/lib/components/Shell.test.ts        | 60 +++++++++++----------
 web/src/routes/discover/+page.svelte        |  5 +-
 web/src/routes/requests/+page.svelte        |  5 +-
 6 files changed, 136 insertions(+), 45 deletions(-)
 create mode 100644 web/src/lib/components/DiscoverTabs.svelte
 create mode 100644 web/src/lib/components/DiscoverTabs.test.ts

diff --git a/web/src/lib/components/DiscoverTabs.svelte b/web/src/lib/components/DiscoverTabs.svelte
new file mode 100644
index 00000000..300a97ea
--- /dev/null
+++ b/web/src/lib/components/DiscoverTabs.svelte
@@ -0,0 +1,36 @@
+<script lang="ts">
+  import { page } from '$app/state';
+
+  type Item = { href: string; label: string };
+
+  // Discover and Requests are paired: discover an artist → request it. The
+  // tab strip lifts both surfaces onto the same horizontal axis so the
+  // operator can move between them without round-tripping through the
+  // main nav.
+  const items: Item[] = [
+    { href: '/discover', label: 'Discover' },
+    { href: '/requests', label: 'Requests' }
+  ];
+
+  function isActive(href: string): boolean {
+    return page.url.pathname.startsWith(href);
+  }
+</script>
+
+<nav aria-label="Discover sections" class="border-b border-border">
+  <ul class="flex gap-2">
+    {#each items as item (item.href)}
+      <li>
+        <a
+          href={item.href}
+          aria-current={isActive(item.href) ? 'page' : undefined}
+          class="block border-b-2 px-3 py-2 text-sm transition-colors {isActive(item.href)
+            ? 'border-accent text-text-primary'
+            : 'border-transparent text-text-secondary hover:text-text-primary'}"
+        >
+          {item.label}
+        </a>
+      </li>
+    {/each}
+  </ul>
+</nav>
diff --git a/web/src/lib/components/DiscoverTabs.test.ts b/web/src/lib/components/DiscoverTabs.test.ts
new file mode 100644
index 00000000..ebcc9e54
--- /dev/null
+++ b/web/src/lib/components/DiscoverTabs.test.ts
@@ -0,0 +1,35 @@
+import { describe, expect, test, vi } from 'vitest';
+import { render, screen } from '@testing-library/svelte';
+
+const state = vi.hoisted(() => ({
+  pageUrl: new URL('http://localhost/discover')
+}));
+
+vi.mock('$app/state', () => ({
+  page: { get url() { return state.pageUrl; } }
+}));
+
+import DiscoverTabs from './DiscoverTabs.svelte';
+
+describe('DiscoverTabs', () => {
+  test('Discover tab is current on /discover', () => {
+    state.pageUrl = new URL('http://localhost/discover');
+    render(DiscoverTabs);
+    expect(screen.getByRole('link', { name: /discover/i })).toHaveAttribute('aria-current', 'page');
+    expect(screen.getByRole('link', { name: /requests/i })).not.toHaveAttribute('aria-current');
+  });
+
+  test('Requests tab is current on /requests', () => {
+    state.pageUrl = new URL('http://localhost/requests');
+    render(DiscoverTabs);
+    expect(screen.getByRole('link', { name: /requests/i })).toHaveAttribute('aria-current', 'page');
+    expect(screen.getByRole('link', { name: /discover/i })).not.toHaveAttribute('aria-current');
+  });
+
+  test('renders both tabs in order', () => {
+    state.pageUrl = new URL('http://localhost/discover');
+    render(DiscoverTabs);
+    const labels = screen.getAllByRole('link').map((l) => l.textContent?.trim());
+    expect(labels).toEqual(['Discover', 'Requests']);
+  });
+});
diff --git a/web/src/lib/components/Shell.svelte b/web/src/lib/components/Shell.svelte
index 9d121751..ddfeb032 100644
--- a/web/src/lib/components/Shell.svelte
+++ b/web/src/lib/components/Shell.svelte
@@ -21,24 +21,17 @@
     return page.url.pathname.startsWith(href);
   }
 
+  // Search reaches /search via the global SearchInput in the header (not the
+  // nav). Requests is a tab inside Discover. Settings + Admin live in the
+  // username dropdown — they're per-operator chrome, not primary surfaces.
   const navItems = [
     { href: '/',                label: 'Home' },
     { href: '/library/artists', label: 'Artists' },
     { href: '/library/albums',  label: 'Albums' },
     { href: '/library/liked',   label: 'Liked' },
-    { href: '/search',          label: 'Search' },
     { href: '/discover',        label: 'Discover' },
-    { href: '/requests',        label: 'Requests' },
-    { href: '/playlists',       label: 'Playlists' },
-    { href: '/settings',        label: 'Settings' }
+    { href: '/playlists',       label: 'Playlists' }
   ];
-
-  // Admin link sits between Playlists and Settings, only visible to admins.
-  const visibleNavItems = $derived(
-    user.value?.is_admin
-      ? [...navItems.slice(0, -1), { href: '/admin', label: 'Admin' }, navItems[navItems.length - 1]]
-      : navItems
-  );
 </script>
 
 <svelte:window onclick={() => (menuOpen = false)} onkeydown={(e) => e.key === 'Escape' && (menuOpen = false)} />
@@ -59,15 +52,34 @@
       </button>
       {#if menuOpen}
         <div
-          class="absolute right-0 mt-1 w-32 rounded border border-border bg-surface shadow"
+          class="absolute right-0 mt-1 w-40 rounded border border-border bg-surface shadow"
           onclick={(e) => e.stopPropagation()}
           onkeydown={(e) => e.stopPropagation()}
           role="menu"
           tabindex="-1"
         >
+          <a
+            href="/settings"
+            role="menuitem"
+            class="block px-3 py-2 text-sm hover:bg-surface-hover"
+            onclick={() => (menuOpen = false)}
+          >
+            Settings
+          </a>
+          {#if user.value?.is_admin}
+            <a
+              href="/admin"
+              role="menuitem"
+              class="block px-3 py-2 text-sm hover:bg-surface-hover"
+              onclick={() => (menuOpen = false)}
+            >
+              Admin
+            </a>
+          {/if}
           <button
             type="button"
-            class="block w-full px-3 py-2 text-left hover:bg-surface-hover"
+            role="menuitem"
+            class="block w-full border-t border-border px-3 py-2 text-left text-sm hover:bg-surface-hover"
             onclick={handleLogout}
           >
             Log out
@@ -79,7 +91,7 @@
 
   <nav class="hidden w-48 border-r border-border bg-surface md:block">
     <ul class="p-2">
-      {#each visibleNavItems as item}
+      {#each navItems as item}
         <li>
           <a
             href={item.href}
diff --git a/web/src/lib/components/Shell.test.ts b/web/src/lib/components/Shell.test.ts
index fbd15374..5771f11b 100644
--- a/web/src/lib/components/Shell.test.ts
+++ b/web/src/lib/components/Shell.test.ts
@@ -39,54 +39,56 @@ describe('Shell', () => {
     expect(screen.getByText('alice')).toBeInTheDocument();
   });
 
-  test('renders nav items in expected order', () => {
+  test('main nav renders the six primary surfaces in order', () => {
     render(Shell);
-    const labels = ['Home', 'Artists', 'Albums', 'Liked', 'Search',
-                    'Discover', 'Requests', 'Playlists', 'Settings'];
+    const labels = ['Home', 'Artists', 'Albums', 'Liked', 'Discover', 'Playlists'];
     for (const label of labels) {
       expect(screen.getByRole('link', { name: label })).toBeInTheDocument();
     }
     expect(screen.getByRole('link', { name: 'Home' })).toHaveAttribute('href', '/');
     expect(screen.getByRole('link', { name: 'Artists' })).toHaveAttribute('href', '/library/artists');
     expect(screen.getByRole('link', { name: 'Albums' })).toHaveAttribute('href', '/library/albums');
-    expect(screen.getByRole('link', { name: 'Search' })).toHaveAttribute('href', '/search');
     expect(screen.getByRole('link', { name: 'Discover' })).toHaveAttribute('href', '/discover');
-    expect(screen.getByRole('link', { name: 'Requests' })).toHaveAttribute('href', '/requests');
     expect(screen.getByRole('link', { name: 'Playlists' })).toHaveAttribute('href', '/playlists');
   });
 
-  test('Hidden link is not in the main nav', () => {
-    render(Shell);
-    expect(screen.queryByRole('link', { name: 'Hidden' })).not.toBeInTheDocument();
-  });
-
-  test('non-admin users do not see the Admin nav link', () => {
-    userState.current = { id: '1', username: 'alice', is_admin: false };
-    render(Shell);
-    expect(screen.queryByRole('link', { name: 'Admin' })).not.toBeInTheDocument();
-  });
-
-  test('admin users see the Admin nav link between Playlists and Settings', () => {
+  test('main nav omits Search, Requests, Hidden, Settings, and Admin', () => {
     userState.current = { id: '1', username: 'alice', is_admin: true };
     render(Shell);
-    const link = screen.getByRole('link', { name: 'Admin' });
-    expect(link).toHaveAttribute('href', '/admin');
-    // Order check: Playlists then Admin then Settings.
-    const labels = screen
+    // Search reaches /search via the global SearchInput, Requests is a
+    // tab inside Discover, Hidden lives under Settings, Settings + Admin
+    // live in the username dropdown — none belong on the main nav.
+    const navLinks = screen
       .getAllByRole('link')
-      .map((el) => el.textContent?.trim())
-      .filter(Boolean);
-    const idxPlaylists = labels.indexOf('Playlists');
-    const idxAdmin = labels.indexOf('Admin');
-    const idxSettings = labels.indexOf('Settings');
-    expect(idxPlaylists).toBeLessThan(idxAdmin);
-    expect(idxAdmin).toBeLessThan(idxSettings);
+      .filter((el) => el.closest('nav') !== null)
+      .map((el) => el.textContent?.trim());
+    for (const label of ['Search', 'Requests', 'Hidden', 'Settings', 'Admin']) {
+      expect(navLinks).not.toContain(label);
+    }
+  });
+
+  test('user-menu shows Settings + Log out for non-admin users', async () => {
+    userState.current = { id: '1', username: 'alice', is_admin: false };
+    render(Shell);
+    await fireEvent.click(screen.getByRole('button', { name: /alice/i }));
+    expect(screen.getByRole('menuitem', { name: 'Settings' })).toHaveAttribute('href', '/settings');
+    expect(screen.getByRole('menuitem', { name: /log out/i })).toBeInTheDocument();
+    expect(screen.queryByRole('menuitem', { name: 'Admin' })).not.toBeInTheDocument();
+  });
+
+  test('user-menu adds Admin between Settings and Log out for admin users', async () => {
+    userState.current = { id: '1', username: 'alice', is_admin: true };
+    render(Shell);
+    await fireEvent.click(screen.getByRole('button', { name: /alice/i }));
+    const items = screen.getAllByRole('menuitem').map((el) => el.textContent?.trim());
+    expect(items).toEqual(['Settings', 'Admin', 'Log out']);
+    expect(screen.getByRole('menuitem', { name: 'Admin' })).toHaveAttribute('href', '/admin');
   });
 
   test('user-menu "Log out" calls logout() and navigates to /login', async () => {
     render(Shell);
     await fireEvent.click(screen.getByRole('button', { name: /alice/i }));
-    await fireEvent.click(screen.getByRole('button', { name: /log out/i }));
+    await fireEvent.click(screen.getByRole('menuitem', { name: /log out/i }));
     expect(logout).toHaveBeenCalledTimes(1);
     expect(goto).toHaveBeenCalledWith('/login', { replaceState: true });
   });
diff --git a/web/src/routes/discover/+page.svelte b/web/src/routes/discover/+page.svelte
index dac4c2b5..d00badf6 100644
--- a/web/src/routes/discover/+page.svelte
+++ b/web/src/routes/discover/+page.svelte
@@ -2,6 +2,7 @@
   import { createLidarrSearchQuery } from '$lib/api/lidarr';
   import { createRequest } from '$lib/api/requests';
   import DiscoverResultCard from '$lib/components/DiscoverResultCard.svelte';
+  import DiscoverTabs from '$lib/components/DiscoverTabs.svelte';
   import ApiErrorBanner from '$lib/components/ApiErrorBanner.svelte';
   import SuggestionFeed from '$lib/components/SuggestionFeed.svelte';
   import type {
@@ -114,7 +115,9 @@
   ];
 </script>
 
-<div class="space-y-6">
+<DiscoverTabs />
+
+<div class="space-y-6 pt-6">
   <input
     type="search"
     aria-label="Search Lidarr"
diff --git a/web/src/routes/requests/+page.svelte b/web/src/routes/requests/+page.svelte
index 3907cc8c..bfb108e2 100644
--- a/web/src/routes/requests/+page.svelte
+++ b/web/src/routes/requests/+page.svelte
@@ -5,6 +5,7 @@
   import { qk } from '$lib/api/queries';
   import StatusPill from '$lib/components/StatusPill.svelte';
   import ApiErrorBanner from '$lib/components/ApiErrorBanner.svelte';
+  import DiscoverTabs from '$lib/components/DiscoverTabs.svelte';
   import type { LidarrRequest, LidarrRequestKind } from '$lib/api/types';
 
   const queryStore = createMyRequestsQuery();
@@ -60,7 +61,9 @@
   }
 </script>
 
-<div class="space-y-6">
+<DiscoverTabs />
+
+<div class="space-y-6 pt-6">
   <header class="space-y-1">
     <h2 class="font-display text-2xl font-medium text-text-primary">
       Your requests