Add a phased improvement plan for the app

docs/improvement-plan.md

@@ -0,0 +1,153 @@
+# ytplayer — Improvement Plan (2026-09-29)
+
+Grounded in the current tree: `frontend/app.js` 9.8k lines / 30+ module objects,
+`styles.css` 4.3k, `server/server.js` 1.9k with ~30 routes, 11 unit/server test
+files (~110 cases), 5 Playwright specs, **no CI**, no lint/format config.
+The feature set is already broad (lyrics, party, remote, presenter, stats,
+transitions, uploads). The biggest returns now come from **hardening and
+maintainability**, not more features.
+
+Priority key: **P0** do next · **P1** this quarter · **P2** when there's room.
+Effort: S ≤ ½ day · M 1–3 days · L a week+.
+
+---
+
+## Phase 1 — Safety net (P0)
+
+Nothing below is safe to refactor without it; the CLAUDE.md history (update
+banner "fixed three times", the thumb-cache opaque trap, the toast-loop tab
+crash) shows regressions have repeatedly shipped silently.
+
+| # | Item | Effort | Done when |
+|---|------|--------|-----------|
+| 1.1 | **GitHub Actions CI**: `node --test frontend/*.test.js`, `cd server && bun run test`, Playwright chromium smoke specs. Run on every push/PR. | S | Red CI blocks a merge; badge in README. |
+| 1.2 | Fix root `npm test` — it runs `node --test frontend/`, which CLAUDE.md says fails on Node 22. Change to `node --test frontend/*.test.js` and add `test:server`, `test:e2e`, `test:all`. | S | `npm run test:all` green locally and in CI. |
+| 1.3 | **Lint + format**: Biome (one binary, zero config sprawl) with `no-unused-vars`, `no-undef`, `useAwait`, `noFallthrough`. Start with `--diagnostic-level=error` so it lands without a mass reformat commit. | S | CI step; existing code passes. |
+| 1.4 | **Regression tests for the known fragile paths**: (a) `/sw.js` BUILD_TAG regex injection survives a bumped fallback literal; (b) `maybeShowUpdateBanner` single-rule behaviour; (c) thumb cache accepts CORS-mode responses (status 0 opaque trap); (d) `toast()` cap with deferred removal terminates. | M | Each has a test that fails on the historical bug. |
+| 1.5 | **Server route contract tests** for `/api/streams`, `/api/search`, `/api/user/sync`, `/api/profile/*` against a stubbed yt-dlp binary (a script that prints fixture JSON). Guards the "JSON shapes mirror the Tauri bridge" rule. | M | Shape snapshot per endpoint. |
+
+## Phase 2 — Security & abuse hardening (P0/P1)
+
+| # | Item | Pri | Effort | Notes |
+|---|------|-----|--------|-------|
+| 2.1 | **Rate-limit expensive routes.** `/api/search`, `/api/channel`, `/api/streams`, `/api/playlist/expand`, `/api/download` each spawn yt-dlp/ffmpeg with no per-IP limit — a trivial loop can exhaust CPU and get the homelab IP bot-gated by YouTube (which already degrades saves to 360p, see BACKLOG). Token bucket per first `X-Forwarded-For` hop, plus a global concurrency cap on yt-dlp spawns with a small queue. | P0 | M | Reuse the IP logic already written for `REMOTE_SAME_NETWORK`. |
+| 2.2 | **Profile name enumeration.** Names are the only credential and may be 3 chars (`PROFILE_NAME_RE`), so `/api/profile/load` + `/api/playlist/inbox` are brute-forceable and nothing throttles them. Minimum: strict per-IP limit on 404s from those routes + raise the minimum for *new* profiles to ~10 chars or suggest a generated passphrase. Optional later: an opt-in per-profile PIN (hash stored) that `save` requires. | P0 | S–M | Keep "name = passkey" UX for existing profiles. |
+| 2.3 | **Security headers** via `hono/secure-headers`: CSP (self + ytimg/ggpht images + the listed CDNs), `X-Content-Type-Options`, `Referrer-Policy`, `frame-ancestors 'self'`. Start CSP in `Report-Only` for a week — app.js builds a lot of `innerHTML` (124 sites). | P1 | M | Admin page gets a stricter policy. |
+| 2.4 | **innerHTML audit**: lint rule / grep gate that every interpolation into `innerHTML` goes through `escapeHtml` (67 call sites today vs 124 innerHTML writes). Convert the risky ones (titles, channel names, chat, notes, profile names from URLs) to `textContent` or a tiny `html\`\`` tagged template that escapes by default. | P1 | M | Party chat + shared playlists carry attacker-controlled text. |
+| 2.5 | Body-size limits on every JSON POST (profile already has `PROFILE_MAX_BYTES`; check `/api/user/sync`, notes, party/remote WS frames). | P1 | S | |
+| 2.6 | Finish the **cookies jar** backlog item (mount `YTDLP_COOKIES` via a compose volume + an admin-page "cookies expire in N days" warning) — only if 360p fallback saves are noticed. | P2 | S | Already documented in BACKLOG. |
+
+## Phase 3 — Break up `app.js` (P1)
+
+9.8k lines in one file is the main drag on every change (and on AI-assisted
+edits, which have to re-read huge ranges). The modules are already
+IIFE-shaped (`Notes`, `Remote`, `Transition`, `Wave`, `Party`, `Presenter`,
+`SectionRail`, `EQ`, …), so the split is mechanical.
+
+1. **Native ES modules, no bundler.** `<script type="module" src="app.js">`,
+   one file per existing module under `frontend/modules/`. No build step keeps
+   the Tauri / zero-native shells and the SW precache simple.
+2. **Order of extraction** (lowest coupling first): `RecentSearches`, `EQ`,
+   `PiP`, `Gestures`, `External`, `Share`, `StatsTrack`, `Wave`,
+   `Transition`, `SectionRail`, `Presenter`, `Remote`, `Party`, `Notes`,
+   then `Player` and the render/sidebar core last.
+3. Shared state (`data`, `queue`, `queueIndex`, `persist()`, `toast()`,
+   `API`) moves to `frontend/modules/state.js` and is imported, not global.
+4. **SW precache list** must be generated, not hand-maintained: have the
+   server (which already walks `./public` for `BUILD_TAG`) inject the shell
+   file list into `/sw.js` alongside the tag. Otherwise a new module file
+   missing from `SHELL` breaks offline — the exact class of bug CLAUDE.md
+   warns about.
+5. Move pure logic out as testable units the way `lyrics-core.js` and
+   `stats-core.js` already are: queue/shuffle math, A-B marker resolution
+   (per-playlist vs fallback), party clock extrapolation, sync merge.
+6. Same treatment for `server/server.js`: split into `routes/search.js`,
+   `routes/streams.js`, `routes/profile.js`, `routes/playlist.js`,
+   `ytdlp.js` (spawn + `runYtdlpResilient`), mirroring how `notes.js` /
+   `uploads.js` already register their routes.
+
+**Done when:** no frontend file > 1.5k lines, no server file > 600, all
+tests + e2e green, SW offline check passes (the real-browser procedure in
+CLAUDE.md → Testing).
+
+## Phase 4 — Reliability & observability (P1)
+
+| # | Item | Effort |
+|---|------|--------|
+| 4.1 | **Structured logs** (JSON lines: route, status, ms, videoId, yt-dlp client used, error class) instead of ad-hoc `console.*`. | S |
+| 4.2 | **`/api/health`** that actually checks: DB writable, `MEDIA_DIR` free space, yt-dlp version + age, last successful extraction time, queue depth. Point the Docker healthcheck at it (today it pings `/api/version`, which stays green while yt-dlp is broken). | S |
+| 4.3 | **yt-dlp staleness**: the image fetches `latest` only at build time, and "a 2-month-old one 403s on every download". Add a nightly in-container self-update (`yt-dlp -U` into the data volume, fall back to the baked binary) or a scheduled rebuild. | S |
+| 4.4 | **Client error reporting**: `window.onerror` + `unhandledrejection` + media element `error` events → throttled `POST /api/client-log` (build tag, UA, route). Surfaces the iOS/HEVC/playback failures that currently only show up when a user complains. Visible in `/admin`. | M |
+| 4.5 | **DB backups**: notes already get a daily JSON dump; do the same (or a `VACUUM INTO` copy) for `profiles`, `users`, `uploads`. Document a restore drill. | S |
+| 4.6 | **Deploy on merge**: a GitHub Action that calls the Dokploy deploy API (key as a repo secret) after CI passes on `main`, then polls `/api/version` for the new `buildTag`. Replaces the manual SSH hop; keep the `deploy-prod` skill as the manual fallback. | M |
+
+## Phase 5 — Performance (P1/P2)
+
+- **First load**: measure with Lighthouse on a throttled mobile profile.
+  `app.js` + `styles.css` are ~14k lines parsed on every cold start; after the
+  Phase 3 split, lazy-`import()` the rarely used modules (Party, Presenter,
+  Remote, EQ, admin-ish editors) on first use.
+- **Server search cache** (3 min, just added) → also cache `/api/streams`
+  metadata (not the signed URLs) and `/api/channel` briefly.
+- **CSS**: split per feature alongside the modules; drop dead selectors
+  (run a coverage pass in Chromium DevTools over the main flows).
+- **DB**: check indexes for the hot queries (`video_history` has one;
+  verify `media_cache.last_access` for LRU, `video_note_revs(video_id, kind)`).
+- **Media cache**: expose hit rate in `/api/media/stats` so the 10 GiB budget
+  and `MEDIA_AUTO_MAX_SECONDS` can be tuned from data.
+
+## Phase 6 — UX & accessibility (P1/P2)
+
+- **Accessibility pass** (only ~40 `aria-` attributes across index.html +
+  styles): labelled icon-only buttons (transport, ↦/⇥/⤨, ⚠ Broken, rail
+  icons), focus-visible styles, `prefers-reduced-motion`, keyboard access to
+  drag-reorder, live-region announcements for toasts and track changes.
+  Add `@axe-core/playwright` to CI on the main screens.
+- **Onboarding**: the app now has many power features that are hard to
+  discover (service mode, presenter, remote, party, transitions, EQ). One
+  "What's new / Tips" sheet tied to the build tag, dismissible.
+- **Settings organisation**: group into Playback · Lyrics & service · Sync &
+  profile · Offline & storage · Advanced; add search within settings.
+- **Offline clarity**: global indicator when offline + which views work,
+  and a storage screen showing OPFS vs thumb cache vs quota.
+- **Profile security UX** (pairs with 2.2): "Your profile name is your
+  password" hint + generate-a-strong-name button.
+
+## Phase 7 — Feature ideas (P2, after 1–4)
+
+Ranked by fit with the worship/service use case:
+1. **Set-list planning**: date-stamped service plans (songs + key + notes +
+   order), shareable read-only link for the band, one-tap into service mode.
+2. **Chord charts** alongside lyrics (ChordPro import, transpose using the
+   existing `@ Key G` tag).
+3. **Per-song key/tempo playback** (pitch-preserving rate already exists;
+   add pitch shift via a Web Audio worklet, desktop only because of iOS).
+4. **Presenter themes** (background image/video, font, safe-area margins)
+   and a stage/confidence-monitor view showing the next line.
+5. **Multi-user admin** (roles instead of one `ADMIN_PASSWORD`).
+
+## Phase 8 — Docs & housekeeping (P2)
+
+- `CLAUDE.md` is excellent but ~400 lines and growing; move per-feature
+  deep dives to `docs/architecture/*.md` and keep CLAUDE.md as the index +
+  the "never do X" rules.
+- README: quickstart, screenshots, env var table (currently only in compose
+  comments).
+- Retire or clearly quarantine the Tauri/zero-native shells if they're no
+  longer shipped — every JSON-shape change currently has to consider them.
+- Version: `APP_VERSION` is hard-coded `1.0.0` in three places; derive it
+  from one source or drop it in favour of `buildTag`.
+
+---
+
+## Suggested order
+
+1. Week 1: 1.1, 1.2, 1.3, 2.1, 2.2, 4.2, 4.3
+2. Week 2: 1.4, 1.5, 2.5, 4.1, 4.4, 4.5
+3. Weeks 3–5: Phase 3 split (server first — smaller and better tested),
+   with 2.3/2.4 riding along as modules move
+4. Then: 4.6 auto-deploy, Phase 5 perf, Phase 6 a11y/UX
+5. Features (Phase 7) once CI + the split are in place
+
+Each item is sized to be one commit per CLAUDE.md's commit rules, and most
+can be queued with the `plan-queue` skill for step-by-step execution.