From 224411bbedc9e1bdf006c09530c5ea560cfd6e0c Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Fri, 6 Mar 2026 10:49:03 -0500 Subject: [PATCH] docs: complete v1.1 project research --- .planning/research/ARCHITECTURE.md | 1442 ++++++++++++++++------------ .planning/research/FEATURES.md | 798 +++++++++------ .planning/research/PITFALLS.md | 558 ++++++----- .planning/research/STACK.md | 1058 +++++++++----------- .planning/research/SUMMARY.md | 267 ++--- 5 files changed, 2261 insertions(+), 1862 deletions(-) diff --git a/.planning/research/ARCHITECTURE.md b/.planning/research/ARCHITECTURE.md index 11d8889..0d09e10 100644 --- a/.planning/research/ARCHITECTURE.md +++ b/.planning/research/ARCHITECTURE.md @@ -1,754 +1,970 @@ -# Architecture Research: Refactoring Patterns for YellowJacket Consolidation +# Architecture Patterns: v1.1 Feature Integration -**Domain:** Go/Wails/Lit desktop music player — codebase consolidation -**Researched:** 2026-02-27 -**Confidence:** HIGH (patterns derived from codebase analysis + Go stdlib + official sqlc docs) +**Domain:** Desktop music player — new feature integration with existing Wails/Lit/beep/SQLite architecture +**Researched:** 2026-03-06 +**Confidence:** HIGH (derived from complete codebase read + official docs for beep, MusicBrainz API, dhowden/tag) -## Issue 1: Two-Phase Initialization Race Conditions +## Recommended Architecture -### Current Problem +YellowJacket v1.1 adds 8 features to an existing, well-structured codebase. The architecture approach is **integration-first**: each feature slots into established patterns (two-phase init, event-driven sync, mutex-protected state, sqlc codegen) rather than introducing new architectural paradigms. The one exception is the plugin system, which necessarily introduces a new extension mechanism. -Six components use a `SetContext(ctx context.Context)` pattern where the Wails runtime context is stored on a struct field without synchronization: +### High-Level Integration Map + +``` + ┌─────────────────────────────────────────┐ + │ app.go (wiring) │ + │ New: TagEditor, SmartPlaylist, │ + │ MusicBrainz, Shortcuts, Layout │ + └────────────────┬────────────────────────┘ + │ + ┌────────────────────────────┼────────────────────────────┐ + │ │ │ + ┌────▼────┐ ┌─────▼─────┐ ┌─────▼─────┐ + │ player │ │ database │ │ events │ + │ │ │ │ │ │ + │ +gapless│ │ +smart_pl │ │ +TagsEdit │ + │ +xfade │ │ +shortcuts│ │ +ScanCanc │ + │ │ │ +layout │ │ +SmartPL │ + └─────────┘ └───────────┘ │ +Shortcut │ + │ +Layout │ + │ +MBrainz │ + └───────────┘ +``` + +### Component Boundaries + +| Component | Responsibility | New vs Modified | Communicates With | +|-----------|---------------|-----------------|-------------------| +| `backend/tageditor/` | Read/write audio file tags, coordinate DB updates | **NEW** package | metadata, database, library, events | +| `backend/library/` | Scan cancellation via context | **MODIFIED** | database, events, coverart | +| `backend/smartplaylist/` | Rule-based dynamic playlists | **NEW** package | database, events | +| `backend/shortcuts/` | Keyboard shortcut registry + dispatch | **NEW** package | config, events, player, queue | +| `backend/player/` | Gapless playback + crossfade | **MODIFIED** | beep, events, queue | +| `backend/musicbrainz/` | MusicBrainz API client + caching | **NEW** package | database (cache tables), events | +| `backend/layout/` | Layout section configuration | **NEW** package | config, events | +| `backend/plugin/` | Plugin loading, lifecycle, API surface | **NEW** package | all packages (via API) | +| `frontend/src/store/tageditor-store.ts` | Tag edit state | **NEW** store | backend tageditor bindings | +| `frontend/src/store/smartplaylist-store.ts` | Smart playlist state | **NEW** store | backend smartplaylist bindings | +| `frontend/src/store/shortcut-store.ts` | Shortcut config state | **NEW** store | backend shortcuts bindings | +| `frontend/src/store/musicbrainz-store.ts` | MB browsing state | **NEW** store | backend musicbrainz bindings | +| `frontend/src/store/layout-store.ts` | Layout config state | **NEW** store | backend layout bindings | + +### Data Flow + +**Existing pattern preserved**: Backend is source of truth. Frontend stores are reactive mirrors. Events flow backend-to-frontend. Actions flow frontend-to-backend via Wails bindings. + +**New data flows:** + +1. **Tag Edit Flow**: Frontend collects edits → Wails binding → `tageditor.SaveTags()` → write file tags → update DB records → emit `TagsEdited` event → frontend refreshes affected views +2. **Scan Cancel Flow**: Frontend sends cancel request → Wails binding → `library.CancelScan()` → cancel context propagation → scan goroutines check `ctx.Done()` → emit `LibraryScanCancelled` event +3. **Smart Playlist Flow**: User defines rules via frontend → Wails binding → `smartplaylist.Create()` → persist rules to DB → evaluate rules → emit `SmartPlaylistChanged` event → frontend refreshes +4. **Gapless Flow**: Player pre-decodes next track in background → when current track ends, swap streamer chains without speaker interruption → seamless transition +5. **MusicBrainz Flow**: Frontend search query → Wails binding → `musicbrainz.Search()` → HTTP GET to MB API (rate-limited) → cache results in SQLite → return to frontend → display + +--- + +## Feature 1: Tag Editing + +### Architecture + +**New package: `backend/tageditor/`** + +The existing `dhowden/tag` library is **read-only**. Tag writing requires a separate library. Use `bogem/id3v2` for MP3 files and `go-flac/go-flac` (or equivalent) for FLAC Vorbis comments. OGG and WAV tag writing can be deferred (LOW priority formats). + +**Confidence:** HIGH — `dhowden/tag` has no write support (confirmed from source). `bogem/id3v2` is the standard Go ID3v2 writer. ```go -// queue/queue.go:134 — no lock -func (q *Queue) SetContext(ctx context.Context) { - q.ctx = ctx +// backend/tageditor/tageditor.go +type TagEditor struct { + mu sync.Mutex + ctx context.Context + logger *slog.Logger + db *database.DB + lib *library.Library // for FTS re-indexing } -// library/library.go:120 — no lock, also calls registerEventHandlers() -func (l *Library) SetContext(ctx context.Context) { - l.ctx = ctx - l.registerEventHandlers() +type TagUpdate struct { + FilePath string + Title string + Artist string + Album string + // ... all editable fields } -// player/player.go:163 — double lock/unlock -func (p *Player) SetContext(ctx context.Context) { - p.mu.Lock() - p.ctx = ctx - p.mu.Unlock() - p.mu.Lock() - p.restoreStateLocked() - p.mu.Unlock() +func (te *TagEditor) SaveTags(update TagUpdate) error { + // 1. Write tags to audio file (format-specific writer) + // 2. Update recording/artist_credit/release_group in DB + // 3. Re-index in FTS5 search_index + // 4. Emit TagsEdited event with affected file paths } ``` -The race is technically real: `q.ctx` is written without `q.mu` but read inside methods that hold `q.mu`. Go's race detector would flag this. In practice it's safe because `SetContext` is called once during sequential startup in `OnStartup()`, before any concurrent access is possible. +### Integration Points -### Recommended Approach: Mutex-Guarded SetContext +| Touchpoint | Change | Risk | +|------------|--------|------| +| `backend/app.go` | Add TagEditor to `FEBindings`, wire in `OnStartup` | LOW — follows existing pattern | +| `backend/events/events.go` | Add `TagsEdited`, `TagEditFailed` events | LOW — codegen handles sync | +| `backend/database/` | New queries: `UpdateRecordingMetadata`, `GetRecordingByAudioFileID` | LOW — sqlc pattern | +| `backend/metadata/tags.go` | Add `WriteTags()` function alongside existing `ExtractTags()` | MEDIUM — new dependency | +| `frontend/src/components/` | New `` component (modal/panel) | LOW | +| Library/Queue/Player stores | Must react to `TagsEdited` to refresh displayed metadata | MEDIUM — cross-store coordination | -**Do NOT use `sync.Once` or `atomic.Value`.** These are the wrong tools because: +### Critical Design Decision -- `sync.Once` is for "do this exactly once" initialization. `SetContext` doesn't need that — it needs "set this value safely." `sync.Once` would prevent re-setting if the context ever changed (unlikely but architecturally constraining). -- `atomic.Value` requires boxing `context.Context` into an `any`, adds `.Load().(context.Context)` type assertions everywhere the context is read, and makes code harder to follow for no real benefit. +**Write tags to file first, then update DB.** If the file write fails, the DB stays consistent. If the DB update fails after file write, the next scan will reconcile. This matches the existing pattern where the filesystem is the primary source and the DB is derived. -**Instead, hold the existing mutex through the entire SetContext operation:** +### DB Update Strategy -```go -// queue/queue.go — recommended fix -func (q *Queue) SetContext(ctx context.Context) { - q.mu.Lock() - defer q.mu.Unlock() +Tag edits must cascade through the normalized schema: +1. Update `recordings.name` (title) +2. Upsert `artist_credit` + `artists` + link table (if artist changed) +3. Upsert `release_groups` (if album changed) +4. Re-link `release_group_recordings` +5. Re-index FTS5 `search_index` - q.ctx = ctx -} +All within a single transaction. The existing `entityCache` pattern from library scanning can be reused for lookups. -// player/player.go — combine the two lock acquisitions -func (p *Player) SetContext(ctx context.Context) { - p.mu.Lock() - defer p.mu.Unlock() +--- - p.ctx = ctx - p.restoreStateLocked() -} -``` +## Feature 2: Scan Cancellation -For **Library** and **Playlist**, which don't have a mutex because they currently have no concurrent access pattern, add one: +### Architecture + +**Modified: `backend/library/library.go`** + +The scan pipeline already uses `l.ctx` for context propagation. Cancellation requires: +1. A dedicated `context.CancelFunc` stored on the Library struct +2. All scan phases checking `ctx.Done()` (most already do via `select` in the walk and worker phases) ```go type Library struct { - mu sync.Mutex // protects ctx and conf - ctx context.Context - // ... rest unchanged + mu sync.Mutex + ctx context.Context + // ... existing fields ... + scanCancel context.CancelFunc // NEW: cancel function for active scan + scanning bool // NEW: flag for active scan } -func (l *Library) SetContext(ctx context.Context) { +func (l *Library) CancelScan() { l.mu.Lock() defer l.mu.Unlock() + if l.scanCancel != nil { + l.scanCancel() + } +} - l.ctx = ctx - l.registerEventHandlers() +func (l *Library) Scan() (*ScanMetrics, error) { + scanCtx, cancel := context.WithCancel(l.ctx) + l.mu.Lock() + l.scanCancel = cancel + l.scanning = true + l.mu.Unlock() + defer func() { + l.mu.Lock() + l.scanCancel = nil + l.scanning = false + l.mu.Unlock() + }() + // ... existing scan code, but use scanCtx instead of l.ctx ... } ``` -**For `SetPlayer()` and `SetRescanHooks()`:** These are also startup-only setters. The simplest correct fix is to guard them with the same mutex. Alternatively, document a "must be called before first use" contract with a comment. The mutex approach is preferred because it eliminates the race detector complaint without requiring callers to understand ordering constraints. +### Integration Points -### `startupErr` Package-Level Variable +| Touchpoint | Change | Risk | +|------------|--------|------| +| `backend/library/library.go` | Add `scanCancel` field, `CancelScan()` method, wrap scan in child context | LOW — surgical change | +| `backend/events/events.go` | Add `LibraryScanCancelled` event | LOW | +| `frontend/src/store/library-store.ts` | Listen for cancelled event, update scan state | LOW | +| Frontend scan progress UI | Add cancel button | LOW | -Move to a field on `YellowJacketApp`: +### Key Constraint + +The scan's DB writer goroutine commits in batches of 50. Cancellation should allow the current batch to complete (don't leave a half-committed transaction). Check `scanCtx.Done()` between batches, not mid-batch. + +--- + +## Feature 3: Smart Playlists + +### Architecture + +**New package: `backend/smartplaylist/`** + +Smart playlists are rule-based queries that dynamically produce track lists. They are **not** persisted as `playlist_tracks` — they're evaluated on demand from the rule definition. ```go -type YellowJacketApp struct { +// backend/smartplaylist/smartplaylist.go +type Service struct { + mu sync.Mutex + ctx context.Context + logger *slog.Logger + db *database.DB +} + +type Rule struct { + Field string // "genre", "year", "artist", "album", "play_count", "date_added" + Operator string // "is", "is_not", "contains", "greater_than", "less_than", "between" + Value string + Value2 string // for "between" operator +} + +type SmartPlaylist struct { + ID int64 + Name string + Rules []Rule + MatchAll bool // AND vs OR + OrderBy string + Limit int +} + +func (s *Service) Evaluate(id int64) ([]Track, error) { + // 1. Load smart playlist rules from DB + // 2. Build SQL WHERE clause from rules + // 3. Query track_metadata VIEW with dynamic conditions + // 4. Return results +} +``` + +### Database Schema + +```sql +-- New table +CREATE TABLE smart_playlists ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL, + rules_json TEXT NOT NULL, -- JSON-encoded []Rule + match_all BOOLEAN NOT NULL DEFAULT 1, + order_by TEXT NOT NULL DEFAULT 'title', + max_tracks INTEGER NOT NULL DEFAULT 0, -- 0 = unlimited + created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP +); +``` + +### Query Generation Strategy + +Rules map to the existing `track_metadata` VIEW columns. Build parameterized WHERE clauses: + +```go +func buildWhereClause(rules []Rule, matchAll bool) (string, []any) { + // Each rule becomes: "column OPERATOR ?" + // Combined with AND (matchAll) or OR (!matchAll) + // All values are parameterized — no SQL injection risk +} +``` + +**Use raw `db.QueryContext()` for dynamic queries** — sqlc cannot generate dynamic WHERE clauses. Document with `// SAFETY:` comments per existing convention. + +### Integration Points + +| Touchpoint | Change | Risk | +|------------|--------|------| +| `backend/app.go` | Add SmartPlaylist service to `FEBindings` | LOW | +| `backend/database/` | New schema for `smart_playlists` table, migration 6 | LOW | +| `backend/events/events.go` | Add `SmartPlaylistChanged`, `SmartPlaylistDeleted` events | LOW | +| Frontend | New `` component, rules builder UI | MEDIUM — most complex frontend work | + +--- + +## Feature 4: Customizable Keyboard Shortcuts + +### Architecture + +**New package: `backend/shortcuts/`** + +Shortcuts are stored in the TOML config and dispatched via Wails events. The backend holds the definitive shortcut map; the frontend registers a global `keydown` listener that sends key combos to the backend for resolution. + +```go +// backend/shortcuts/shortcuts.go +type Service struct { + mu sync.Mutex + ctx context.Context + logger *slog.Logger + bindings map[string]string // key combo → action name + actions map[string]func() // action name → handler +} + +type Shortcut struct { + Action string `toml:"Action" json:"action"` + Key string `toml:"Key" json:"key"` // e.g., "Ctrl+Space", "MediaPlayPause" +} +``` + +### Config Integration + +Add a `[Shortcuts]` section to `config.toml`: + +```toml +[Shortcuts] +PlayPause = "Space" +NextTrack = "Ctrl+Right" +PrevTrack = "Ctrl+Left" +VolumeUp = "Ctrl+Up" +VolumeDown = "Ctrl+Down" +# ... +``` + +### Frontend Dispatch Pattern + +```typescript +// Frontend: global keydown handler +document.addEventListener('keydown', (e) => { + const combo = buildComboString(e); // e.g., "Ctrl+Space" + Shortcuts.Execute(combo); // Wails binding → backend resolves + executes +}); +``` + +**Why backend dispatch?** The backend already owns all action handlers (player.Play, queue.Next, etc.). Having the backend resolve shortcuts avoids duplicating action dispatch logic in the frontend. The frontend's only job is translating DOM KeyboardEvents into combo strings. + +### Integration Points + +| Touchpoint | Change | Risk | +|------------|--------|------| +| `backend/config/config.go` | Add `Shortcuts` config section | LOW | +| `backend/app.go` | Wire Shortcuts service, register action handlers | LOW | +| `frontend/index.ts` | Add global keydown listener | LOW | +| MPRIS callbacks | Already wired in `app.go OnStartup` — shortcut actions reuse same handler functions | LOW | + +--- + +## Feature 5: Gapless Playback + Crossfade + +### Architecture + +**Modified: `backend/player/player.go`** + +This is the most architecturally complex feature because it fundamentally changes how track transitions work. + +### Gapless Playback + +**Current behavior:** `beep.Callback` fires → `onPlaybackFinished()` goroutine → queue calls `player.LoadFile()` → decode + resample + register with speaker. This gap (file open + decode) causes audible silence. + +**Gapless approach:** Pre-decode the next track while the current one is still playing. When the current track's streamer is near exhaustion, seamlessly swap to the pre-decoded next track. + +```go +type Player struct { // ... existing fields ... - startupErr error // set in OnStartup, checked in OnDomReady + + // Gapless pre-loading + nextFile *os.File + nextStreamer beep.StreamSeekCloser + nextFormat beep.Format + nextBuffered *BufferedStreamer + nextFilePath string + gaplessEnabled bool +} + +// PreloadNext is called by the queue when it knows what track comes next. +func (p *Player) PreloadNext(filePath string) error { + p.mu.Lock() + defer p.mu.Unlock() + // Open file, decode, build resampled chain, store in next* fields + // Do NOT register with speaker yet } ``` -This is safe because Wails guarantees `OnStartup` completes before `OnDomReady` runs — they are sequentially called lifecycle hooks, not concurrent. +**Speaker integration:** Use `beep.Seq()` to chain current + next streamer, or use a custom `GaplessStreamer` that automatically drains the current streamer and transitions to the next one without the `beep.Callback` → goroutine → LoadFile delay. -### Risk Assessment +The key insight: beep's `Seq(a, b)` already provides gapless transition between two streamers. The challenge is having `b` ready before `a` ends. -| Change | Risk | Rationale | -|--------|------|-----------| -| Add mutex to Queue/Config SetContext | **Low** | Mechanical — add lock/unlock, no logic change | -| Combine Player double-lock | **Low** | Reducing lock operations, equivalent behavior | -| Add mutex to Library/Playlist | **Low** | New mutex, but only guards startup path | -| Move startupErr to struct | **Very Low** | Field move, identical semantics | +### Crossfade -### Dependencies - -None — this can be done at any time and is a prerequisite for safe testing of these packages. - ---- - -## Issue 2: Event Name Synchronization - -### Current Problem - -`backend/events/events.go` defines 19 event name constants. `frontend/src/events.ts` mirrors them as an `as const` object. A typo in either file silently breaks communication with no compile-time or runtime detection. - -The TypeScript file is missing `LibraryConfigChanged` from the Go side (it's in the Config events group in Go but absent from the TS events). This is exactly the class of bug this pattern creates. - -### Recommended Approach: Build-Time Code Generation - -**Generate the TypeScript file from the Go source as part of the build.** - -Create a `cmd/genevents/main.go` that parses `backend/events/events.go` using `go/ast` and generates `frontend/src/events.ts`: +Crossfade uses beep's `Mixer` to overlap two tracks: ```go -// cmd/genevents/main.go -package main +// When crossfade is enabled and we're N seconds from track end: +// 1. Start fading out current track's volume +// 2. Start next track at low volume, fade in +// 3. Mix both through beep.Mixer +``` -import ( - "go/ast" - "go/parser" - "go/token" - "os" - "text/template" -) +This requires: +1. A `crossfadeDuration` config setting (default 0 = disabled, range 1-12 seconds) +2. A crossfade mixer that handles the volume ramping +3. Knowing the remaining duration of the current track to trigger crossfade at the right time -const tmpl = `// Code generated by cmd/genevents. DO NOT EDIT. +### Queue Integration -export const Events = { -{{- range .}} - {{.Name}}: "{{.Value}}", -{{- end}} -} as const; +The queue must tell the player what's next: -export type EventName = (typeof Events)[keyof typeof Events]; -` - -func main() { - fset := token.NewFileSet() - f, _ := parser.ParseFile(fset, "backend/events/events.go", nil, 0) - - var events []struct{ Name, Value string } - - ast.Inspect(f, func(n ast.Node) bool { - vs, ok := n.(*ast.ValueSpec) - if !ok || len(vs.Names) == 0 || len(vs.Values) == 0 { - return true - } - bl, ok := vs.Values[0].(*ast.BasicLit) - if !ok { - return true - } - name := vs.Names[0].Name - value := bl.Value[1 : len(bl.Value)-1] // strip quotes - events = append(events, struct{ Name, Value string }{name, value}) - return true - }) - - t := template.Must(template.New("").Parse(tmpl)) - out, _ := os.Create("frontend/src/events.ts") - defer out.Close() - t.Execute(out, events) +```go +// In queue.go, after track advance logic: +func (q *Queue) notifyNextTrack() { + nextIdx := q.peekNextIndex() // look ahead without advancing + if nextIdx >= 0 && nextIdx < len(q.tracks) { + q.player.PreloadNext(q.tracks[nextIdx].FilePath) + } } ``` -Wire into the existing `go generate ./...` pipeline via a directive in `events.go`: +This notification happens: +- After `SetQueue` (next track is known) +- After `Next`/`Previous` (new next track) +- After `OnPlaybackFinished` auto-advance (next-next track) + +### TrackLoader Interface Change ```go -//go:generate go run ../../cmd/genevents/main.go -package events +type TrackLoader interface { + LoadFile(filePath string) error + Play() error + IsPlaying() bool + CurrentPositionSeconds() (int, error) + UnloadTrack() + PreloadNext(filePath string) error // NEW +} ``` -**Why not a shared JSON/YAML schema?** It adds a third file and a parsing step for both sides. Go's AST parsing is trivial and keeps the Go file as the single source of truth. +### Integration Points -**Why not runtime validation?** It would only catch mismatches when the specific event fires, and by then the damage is done. Build-time generation prevents mismatches entirely. +| Touchpoint | Change | Risk | +|------------|--------|------| +| `backend/player/player.go` | Pre-loading, gapless streamer chain, crossfade mixer | **HIGH** — core audio pipeline | +| `backend/queue/queue.go` | `TrackLoader` interface extension, next-track notification | MEDIUM | +| `backend/config/config.go` | Crossfade duration setting | LOW | +| `backend/events/events.go` | Potentially `CrossfadeStarted` event | LOW | +| Speaker initialization | May need larger speaker buffer for crossfade overlap | MEDIUM | -**Build verification step:** Add a `make` target or pre-commit hook check: +### Risk Mitigation -```makefile -check-events: - go generate ./backend/events/... - git diff --exit-code frontend/src/events.ts || (echo "events.ts is out of date" && exit 1) -``` - -### Risk Assessment - -| Change | Risk | Rationale | -|--------|------|-----------| -| Code generator | **Low** | Additive — doesn't change existing code behavior | -| Build integration | **Very Low** | Existing `go generate` pipeline | -| Pre-commit check | **Very Low** | Fails fast if someone edits Go constants without regenerating | - -### Dependencies - -None — independent of all other changes. +- **Start with gapless only**, defer crossfade. Gapless is the higher-value feature. +- Gapless can be implemented by pre-decoding and using `beep.Seq()` to chain streamers — this is well-supported by beep. +- Crossfade is additive — build it on top of working gapless. +- The existing `BufferedStreamer` read-ahead pattern provides a foundation for pre-loading. --- -## Issue 3: Store Architecture for Large Datasets +## Feature 6: MusicBrainz Browser -### Current Problem +### Architecture -`LibraryStore` eagerly calls `GetAllTracks()`, `GetAllAlbums()`, `GetAllArtists()`, `GetAllGenres()` on construction (line 300-304). For a 50k+ track library, this loads all data into the webview's JS heap at startup. +**New package: `backend/musicbrainz/`** -The store already has correct lazy-load infrastructure (check `tracks !== null`, loading flags, `waitFor*` methods). The problem is that `eagerFetch()` bypasses all of it by calling all four getters immediately. +Read-only catalog browsing. The MusicBrainz API is rate-limited to **1 request per second** and requires a meaningful User-Agent header. -### Recommended Approach: Lazy Loading by Active View +**Confidence:** HIGH — MusicBrainz API docs confirmed. JSON format via `fmt=json` or `Accept: application/json`. -The fix is surgical — the infrastructure is already there: +```go +// backend/musicbrainz/client.go +type Client struct { + mu sync.Mutex + ctx context.Context + logger *slog.Logger + db *database.DB // for response caching + httpClient *http.Client + rateLimiter *time.Ticker // 1 req/sec + userAgent string +} -**Step 1: Remove `eagerFetch()` from constructor.** Change the constructor to only set up event listeners: +const apiBaseURL = "https://musicbrainz.org/ws/2/" + +func (c *Client) SearchArtist(query string, limit, offset int) (*ArtistSearchResult, error) +func (c *Client) GetArtist(mbid string) (*Artist, error) +func (c *Client) GetArtistReleaseGroups(mbid string, limit, offset int) (*ReleaseGroupBrowse, error) +func (c *Client) GetReleaseGroup(mbid string) (*ReleaseGroup, error) +func (c *Client) GetRelease(mbid string) (*Release, error) +``` + +### Rate Limiting + +```go +// Enforce 1 request per second globally +func (c *Client) doRequest(url string) ([]byte, error) { + <-c.rateLimiter.C // Block until rate limit allows + // ... execute HTTP GET with User-Agent header ... +} +``` + +### Response Caching + +Cache MB API responses in SQLite to avoid redundant requests: + +```sql +CREATE TABLE musicbrainz_cache ( + url TEXT PRIMARY KEY, + response_json TEXT NOT NULL, + fetched_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP +); +``` + +Cache TTL: 24 hours for search results, 7 days for entity lookups (data changes infrequently). + +### Integration Points + +| Touchpoint | Change | Risk | +|------------|--------|------| +| `backend/app.go` | Add MusicBrainz client to `FEBindings` | LOW | +| `backend/database/` | New `musicbrainz_cache` table, migration | LOW | +| `go.mod` | No new dependencies — use `net/http` from stdlib | LOW | +| Frontend | New `` component with search, artist, album views | MEDIUM | + +### Key Constraint + +The app is currently fully offline. MusicBrainz browsing introduces the first network dependency. Handle network errors gracefully — cache-first with fallback, clear "offline/error" states in the UI, timeouts on HTTP requests. + +--- + +## Feature 7: Layout Customization System + +### Architecture + +**New package: `backend/layout/`** + +MusicBee-style section-based UI customization. The layout defines which component renders in each section of the UI. + +### Section Model + +The current `index.html` defines a fixed layout: +``` +┌─────────────────────────────────────────┐ +│ header (title + search-bar) │ +├─────────┬───────────────────┬───────────┤ +│ sidebar │ main-panel │ queue- │ +│ │ (track-list) │ panel │ +│ │ │ │ +├─────────┴───────────────────┴───────────┤ +│ footer (now-playing + audio-player) │ +└─────────────────────────────────────────┘ +``` + +Make sections configurable: + +```go +// backend/layout/layout.go +type Section struct { + ID string `toml:"ID" json:"id"` + Component string `toml:"Component" json:"component"` // component tag name + Visible bool `toml:"Visible" json:"visible"` +} + +type Layout struct { + Sections []Section `toml:"Sections" json:"sections"` +} +``` + +### Config Integration + +```toml +[Layout] +[[Layout.Sections]] +ID = "sidebar" +Component = "app-sidebar" +Visible = true + +[[Layout.Sections]] +ID = "main-panel" +Component = "track-list" +Visible = true + +[[Layout.Sections]] +ID = "right-panel" +Component = "queue-panel" +Visible = true +``` + +### Frontend Implementation + +The layout engine lives in the frontend. It reads the layout config and dynamically instantiates components in their designated sections: ```typescript -constructor() { - EventsOn(Events.LibraryScanComplete, () => { - this.invalidate(); - }); - this.loadCoverSize(); - // Remove: this.eagerFetch(); +// frontend/src/layout/layout-engine.ts +class LayoutEngine { + private sectionMap: Map; + + applyLayout(config: LayoutConfig) { + for (const section of config.sections) { + const container = this.sectionMap.get(section.id); + if (container) { + container.innerHTML = ''; + if (section.visible) { + const el = document.createElement(section.component); + container.appendChild(el); + } + } + } + } } ``` -**Step 2: Make `invalidate()` only clear caches, not re-fetch:** +### Component Registry + +Each component declares its size constraints (min width, preferred width, etc.) so the layout engine can validate configurations: ```typescript -private invalidate(): void { - this.tracks = null; - this.albums = null; - this.artists = null; - this.genres = null; - this.scrollPositions = { tracks: 0, albums: 0, artists: 0, genres: 0 }; - this.notify(); - // Remove: this.eagerFetch(); +interface LayoutComponent { + tagName: string; + displayName: string; + allowedSections: string[]; // which sections this can go in + minWidth?: number; + minHeight?: number; } + +const COMPONENT_REGISTRY: LayoutComponent[] = [ + { tagName: 'track-list', displayName: 'Track List', allowedSections: ['main-panel'] }, + { tagName: 'cover-grid', displayName: 'Album Grid', allowedSections: ['main-panel'] }, + { tagName: 'queue-panel', displayName: 'Queue', allowedSections: ['right-panel', 'main-panel'] }, + // ... +]; ``` -Data will be fetched on-demand when a view's controller calls `getTracks()`, `getAlbums()`, etc. The existing null-check + loading-flag + waitFor pattern handles concurrent access correctly. +### Integration Points -**Step 3: Prefetch the initial view data only.** If the app opens to the tracks view by default, the tracks controller will trigger `getTracks()` on its first render. This is already what happens — the eager fetch just front-loads all four queries unnecessarily. - -**Step 4 (optional, for 100k+ libraries): Implement paginated data providers.** This is a larger change and should only be pursued if lazy loading alone doesn't solve perceived startup lag. The approach: - -- Backend: Add `GetTracksPage(offset, limit int)` and `GetTrackCount()` queries to sqlc -- Frontend: Replace `library.Track[]` with a `DataProvider` interface that the virtual scroller queries by range -- The existing virtual scrolling components (`track-list`, `cover-grid`) already render only visible rows — they just hold the full dataset backing array - -**Recommendation:** Start with Steps 1-3 (remove eager fetch). Measure. Only build Step 4 if data shows the full `GetAllTracks()` call is still a problem for the initial view. For 50k tracks, a single indexed query returning rows is fast (~100ms on SSD); the bigger cost is JSON serialization across the Wails bridge, which lazy loading solves by deferring non-active-view data. - -### Risk Assessment - -| Change | Risk | Rationale | -|--------|------|-----------| -| Remove eagerFetch | **Low** | Lazy infrastructure already exists and is tested by the `getTracks()` pattern | -| Invalidate without re-fetch | **Low** | Controllers already call getters on update | -| Paginated data providers | **Medium** | Requires backend + frontend + virtual scroller changes | - -### Dependencies - -- Independent of backend changes. -- If paginated data providers are needed, requires new sqlc queries (connects to Issue 5). +| Touchpoint | Change | Risk | +|------------|--------|------| +| `backend/config/config.go` | Add `Layout` config section | LOW | +| `frontend/index.html` | Replace hard-coded components with section containers | MEDIUM | +| `frontend/index.ts` | Initialize layout engine, apply config | MEDIUM | +| All existing components | No changes — they're already self-contained Web Components | LOW | --- -## Issue 4: Queue Persistence — Incremental Updates +## Feature 8: Plugin System -### Current Problem +### Architecture -`commitMutation()` → `persistTracks()` does `DELETE FROM queue_tracks` + batch INSERT for the entire queue on every single mutation (add, remove, move, clear). For a 5000-track queue, every track add triggers a full table rewrite: ~5000 DELETEs + ~5000 INSERTs. +**New package: `backend/plugin/`** -The sqlc queries already define `InsertQueueTrack`, `RemoveQueueTrack`, `RemoveQueueTrackByPosition`, `ShiftQueuePositionsDown`, and `ShiftQueuePositionsUp` — but none of them are used. The persistence layer bypasses sqlc entirely with hand-crafted batch SQL. +This is the most complex architectural addition. The plugin system provides extensibility hooks for both backend logic and frontend UI. -### Recommended Approach: Operation-Specific Persistence +### Plugin Loading -Replace the single `persistTracks()` call with operation-specific methods: +Plugins are directories in `~/.local/share/yellowjacket/plugins/`, each containing: +- `manifest.json` — name, version, entry points, permissions +- `main.js` — frontend code (Lit component) +- `backend.go` (optional) — Go plugin via `plugin` package or WASM -**For AddTrack/AddTracks:** INSERT only the new tracks. +**Recommended approach for v1.1: JavaScript-only plugins.** Go's `plugin` package has severe limitations (Linux-only, same Go version required, no unloading). WASM is possible but adds complexity. JS-only plugins can: +- Register new UI components +- Subscribe to events +- Call exposed backend APIs via Wails bindings +- Add sidebar items, context menu entries, toolbar buttons -```go -func (q *Queue) persistAddTracks(tracks []Track) { - for _, t := range tracks { - _, err := q.db.Queries.InsertQueueTrack(q.db.Ctx, sqlcgen.InsertQueueTrackParams{ - AudioFileID: t.AudioFileID, - Position: t.Position, - }) - if err != nil { - q.logger.Error("Failed to persist added track", "err", err) - } - } +### Plugin API Surface + +```typescript +// frontend/src/plugin/api.ts +interface YellowJacketAPI { + // Events + on(event: string, callback: Function): void; + emit(event: string, data: any): void; + + // Player + player: { + play(): void; + pause(): void; + seek(seconds: number): void; + getState(): PlayerState; + }; + + // Queue + queue: { + addTrack(path: string): void; + getState(): QueueState; + }; + + // Library + library: { + search(query: string): Promise; + getTrackMetadata(path: string): Promise; + }; + + // UI + ui: { + registerSidebarItem(item: SidebarItem): void; + registerContextMenuItem(item: ContextMenuItem): void; + registerComponent(tagName: string, component: typeof LitElement): void; + }; } ``` -**For RemoveTrack/RemoveTracks:** DELETE specific rows + shift positions. +### Plugin Lifecycle + +``` +1. App startup → scan plugins directory +2. Parse manifest.json for each plugin +3. Validate permissions +4. Load JS entry point in sandboxed context +5. Call plugin.init(api) with the API surface +6. Plugin registers its components/handlers +7. App shutdown → call plugin.destroy() for each +``` + +### Sandboxing + +Plugins run in the same webview context (no iframe sandbox — too restrictive for Lit components). Instead, use an API-mediated approach: plugins can only interact with the app through the provided API object, not by reaching into internal stores or DOM directly. + +### Backend Plugin Hooks + +For backend extensibility, expose hooks rather than full plugin loading: ```go -func (q *Queue) persistRemoveTracks(positions []int) { - tx, err := q.db.BeginTx() - if err != nil { return } - txQ := q.db.Queries.WithTx(tx) - - // Remove in descending order to avoid position shifts during removal - slices.SortFunc(positions, func(a, b int) int { return b - a }) - for _, pos := range positions { - txQ.RemoveQueueTrackByPosition(q.db.Ctx, int64(pos)) - txQ.ShiftQueuePositionsDown(q.db.Ctx, int64(pos)) - } - tx.Commit() +// backend/plugin/hooks.go +type Hooks struct { + OnTrackChanged []func(trackInfo player.TrackInfo) + OnLibraryScanDone []func(metrics *library.ScanMetrics) + OnConfigChanged []func(key string, value any) } ``` -**For MoveQueueTracks/InsertNextTracks:** These reorder arbitrary ranges. Use DELETE + INSERT for the affected range only, or fall back to full rewrite when >50% of tracks are affected. +### Integration Points -**For SetQueue and Clear:** Keep the existing DELETE ALL + batch INSERT — these are full replacement operations by definition. +| Touchpoint | Change | Risk | +|------------|--------|------| +| `backend/app.go` | Plugin loader initialization | MEDIUM | +| `backend/plugin/` | New package with loader, manifest parser, hook registry | HIGH — significant new code | +| `frontend/src/plugin/` | New directory with API, loader, registry | HIGH | +| `frontend/index.ts` | Plugin initialization after DOM ready | MEDIUM | +| Security | Plugin code is untrusted — API surface must be carefully scoped | HIGH | -**Refactored `commitMutation`:** +### v1.1 Scope Recommendation -```go -type mutationKind int -const ( - mutationFull mutationKind = iota // SetQueue, Clear - mutationAdd // AddTrack, AddTracks - mutationRemove // RemoveTrack, RemoveTracks - mutationReorder // MoveQueueTracks, InsertNext* -) +For v1.1, implement the **foundation**: +1. Plugin directory scanning + manifest parsing +2. JS plugin loading mechanism +3. Core API surface (events, player, queue) +4. One example plugin demonstrating the pattern -func (q *Queue) commitMutation(kind mutationKind, affectedTracks []Track, affectedPositions []int) { - if q.shuffleMode { - q.generateShuffleOrder() - } - - switch kind { - case mutationAdd: - q.persistAddTracks(affectedTracks) - case mutationRemove: - q.persistRemoveTracks(affectedPositions) - case mutationReorder, mutationFull: - q.persistTracks() // full rewrite for complex operations - } - - q.persistState() -} -``` - -**Performance impact:** For the common case (user adds a track to a 5000-track queue), this goes from ~10,000 SQL operations to 1 INSERT + 1 UPDATE. The full rewrite is reserved for SetQueue (infrequent) and complex reorders. - -### Risk Assessment - -| Change | Risk | Rationale | -|--------|------|-----------| -| Incremental add persistence | **Low** | Uses existing sqlc queries already defined | -| Incremental remove persistence | **Low** | Uses existing sqlc queries + transaction | -| Full rewrite for reorder | **Very Low** | Keeps current behavior for complex cases | -| commitMutation refactor | **Medium** | Changes call signatures throughout queue.go | - -### Dependencies - -- **Should come after Issue 1** (SetContext fixes) so tests can verify persistence correctness. -- **Should come after Issue 6** (test architecture) because persistence changes need test coverage to verify correctness. +Defer to later: backend Go plugins, WASM plugins, plugin marketplace, permissions system. --- -## Issue 5: SQL Query Consolidation — FTS5 JOIN Pattern +## Patterns to Follow -### Current Problem +### Pattern 1: Two-Phase Initialization for New Packages -The same JOIN pattern (audio_files → recordings → artist_credit → release_group_recordings → release_groups) appears in: +**What:** All new packages that need Wails runtime follow `New*()` + `SetContext()`. -1. `SearchFTS()` — search.go:34-57 -2. `SearchFTSByFilename()` — search.go:92-116 -3. `SearchFTSTracks()` — search.go:232-274 -4. `RebuildSearchIndex()` — search.go:168-188 -5. `migration2BasenameAndFTS()` — database.go:287-311 - -Plus a simpler variant in `lookupChunk()` (persistence.go:64-73). - -### Recommended Approach: SQLite VIEW + sqlc Queries - -**Create a VIEW that encapsulates the common JOIN pattern:** - -```sql --- sql/schemas/31_views.sql -CREATE VIEW IF NOT EXISTS track_metadata AS -SELECT - af.id AS audio_file_id, - af.file_path, - af.length_milliseconds, - af.basename, - af.sample_rate, - af.bit_depth, - af.channels, - af.bitrate, - af.file_size, - af.file_type_id, - af.recording_id, - COALESCE(r.name, '') AS title, - COALESCE(ac.text, '') AS artist_name, - r.track_number, - r.disc_number, - COALESCE(r.year, 0) AS year, - COALESCE(r.composer, '') AS composer, - COALESCE(rg.name, '') AS album, - r.artist_credit_id, - r.id AS recording_row_id -FROM audio_files af -LEFT JOIN recordings r ON af.recording_id = r.id -LEFT JOIN artist_credit ac ON r.artist_credit_id = ac.id -LEFT JOIN ( - SELECT recording_id, - MIN(release_group_id) AS release_group_id - FROM release_group_recordings - GROUP BY recording_id -) rgr ON r.id = rgr.recording_id -LEFT JOIN release_groups rg ON rgr.release_group_id = rg.id; -``` - -**Then use the VIEW in sqlc queries:** - -```sql --- sql/queries/search.sql - --- name: SearchFTS :many -SELECT tm.file_path, tm.length_milliseconds, tm.title, tm.artist_name, tm.album -FROM search_index si -JOIN track_metadata tm ON tm.audio_file_id = si.rowid -WHERE search_index MATCH ? -ORDER BY rank -LIMIT ?; - --- name: SearchFTSByFilename :many -SELECT tm.file_path, tm.length_milliseconds, tm.title, tm.artist_name, tm.album -FROM search_index si -JOIN track_metadata tm ON tm.audio_file_id = si.rowid -WHERE search_index MATCH ? -ORDER BY rank -LIMIT ?; - --- name: RebuildSearchIndex :exec -INSERT INTO search_index(rowid, file_path, title, artist, album) -SELECT audio_file_id, file_path, title, artist_name, album -FROM track_metadata; -``` - -**Why a VIEW and not a Go constant/query builder?** -- sqlc can parse VIEWs and generate type-safe Go code from queries against them. -- The JOIN is executed by SQLite's query planner, which optimizes VIEW queries the same as inline JOINs. -- It eliminates all 5 copies of the JOIN at the SQL level, not just the Go level. -- A Go string constant containing the JOIN clause would still require hand-crafted SQL around it, defeating sqlc's type safety. - -**For `lookupChunk` in queue persistence:** This uses `sqlc.slice()` — migrate to: - -```sql --- name: LookupTrackMetaBatch :many -SELECT af.id, af.file_path, - COALESCE(r.name, '') AS title, - COALESCE(ac.text, '') AS artist -FROM audio_files af -LEFT JOIN recordings r ON af.recording_id = r.id -LEFT JOIN artist_credit ac ON r.artist_credit_id = ac.id -WHERE af.file_path IN (sqlc.slice('filePaths')); -``` - -This replaces the hand-crafted `fmt.Sprintf` batch query with sqlc-generated code that handles the dynamic IN clause expansion. Confirmed: sqlc `sqlc.slice()` is supported for MySQL and SQLite (verified in official docs at `docs.sqlc.dev/en/stable/howto/select.html`). - -**For `SearchFTSTracks` (the 16-column variant):** This query has additional columns (genre via subquery, file_type). Extend the VIEW or create a second wider VIEW `track_metadata_full` that includes genre and file_type JOINs. - -**Migration note:** The `migration2BasenameAndFTS` function uses the JOIN inline in a migration. Migrations should NOT reference VIEWs because the VIEW might not exist yet when the migration runs. Keep the inline JOIN in migrations — they run once and don't need deduplication. - -### Risk Assessment - -| Change | Risk | Rationale | -|--------|------|-----------| -| CREATE VIEW | **Low** | SQLite VIEWs are well-supported, IF NOT EXISTS is safe | -| Migrate search to sqlc | **Medium** | Changing hand-crafted SQL to generated code requires careful testing | -| sqlc.slice for batch lookups | **Medium** | Different code generation pattern, needs verification | -| Keep inline JOIN in migrations | **Very Low** | No change to migration code | - -### Dependencies - -- **Should come after Issue 6** (test architecture) so search behavior can be regression-tested. -- Independent of Issues 1-4. - ---- - -## Issue 6: Test Architecture for DB-Dependent Packages - -### Current Problem - -No tests exist for queue, library, database, or config packages. Existing tests (`playlist/match_test.go`, `metadata/*_test.go`) test pure functions that don't require DB or OS dependencies. The player test requires hardware and is skipped in CI. - -### Recommended Approach: In-Memory SQLite + Test Helpers - -**Core test helper — `testdb` package:** +**When:** Any new struct that emits events or uses Wails dialogs. +**Example:** ```go -// internal/testdb/testdb.go -package testdb - -import ( - "testing" - "yellowjacket/backend/database" -) - -// New creates a fresh in-memory database with all schemas applied. -// The database is automatically closed when the test completes. -func New(t *testing.T) *database.DB { - t.Helper() - - db, err := database.NewTestDB() - if err != nil { - t.Fatalf("failed to create test database: %v", err) - } - - t.Cleanup(func() { - db.Close() - }) - - return db -} -``` - -**Modify `database.NewDB` to support in-memory mode:** - -```go -// database/database.go - -// NewTestDB creates an in-memory database for testing. -// It applies all schemas and migrations identically to NewDB. -func NewTestDB() (*DB, error) { - return newDB(":memory:") -} - -// Extract common init logic into newDB(dsn string) -func newDB(dsn string) (*DB, error) { - dbCtx := context.Background() - db, err := sql.Open("sqlite", dsn+"?_busy_timeout=5000&_journal_mode=WAL") - // ... rest of current NewDB logic -} -``` - -The `modernc.org/sqlite` driver fully supports `:memory:` databases. Each test gets an isolated database — no cleanup needed, no file I/O, no disk contention. - -**Test pattern for Queue:** - -```go -// queue/queue_test.go -package queue_test - -import ( - "context" - "testing" - "log/slog" - - "yellowjacket/backend/queue" - "yellowjacket/internal/testdb" -) - -// mockPlayer implements queue.TrackLoader for tests -type mockPlayer struct { - loaded string - playing bool - position int -} - -func (m *mockPlayer) LoadFile(path string) error { m.loaded = path; return nil } -func (m *mockPlayer) Play() error { m.playing = true; return nil } -func (m *mockPlayer) IsPlaying() bool { return m.playing } -func (m *mockPlayer) CurrentPositionSeconds() (int, error) { return m.position, nil } -func (m *mockPlayer) UnloadTrack() { m.loaded = ""; m.playing = false } - -func TestSetQueueAndNavigate(t *testing.T) { - db := testdb.New(t) - - // Seed test tracks - seedTracks(t, db, 10) - - q := queue.NewQueue(slog.Default(), db) - q.SetContext(context.Background()) // no Wails runtime needed for tests - q.SetPlayer(&mockPlayer{}) - - paths := getTestTrackPaths(t, db) - q.SetQueue(paths, 0, false) - - state := q.GetState() - if state.CurrentIndex != 0 { t.Errorf("expected index 0, got %d", state.CurrentIndex) } - if len(state.Tracks) != 10 { t.Errorf("expected 10 tracks, got %d", len(state.Tracks)) } -} -``` - -**Key insight: `context.Background()` works for SetContext in tests.** The Wails context is only needed for `runtime.EventsEmit()` and `runtime.EventsOn()`. In tests, these calls will simply no-op (emit to nobody, subscribe to nobody). Queue logic doesn't depend on event delivery — it just fires and forgets. If a test needs to verify events were emitted, introduce an `EventEmitter` interface later. - -**Test pattern for Config:** - -```go -// config/config_test.go -func TestLoadSaveRoundtrip(t *testing.T) { - dir := t.TempDir() - // Write a known TOML file - // Load it - // Verify fields - // Save it - // Load again - // Verify identical -} -``` - -Config tests don't need a database — they need a temp directory for the TOML file. Use `t.TempDir()`. - -**Test pattern for Database/Search:** - -```go -func TestSearchFTS(t *testing.T) { - db := testdb.New(t) - seedTracksWithMetadata(t, db) - - results, err := db.SearchFTS("beethoven", 10) - if err != nil { t.Fatal(err) } - if len(results) != 1 { t.Errorf("expected 1 result, got %d", len(results)) } -} -``` - -**Test pattern for Player (pure logic extraction):** - -```go -// player/volume_test.go — no hardware needed -func TestUserVolumeToInternal(t *testing.T) { - tests := []struct{ user UserVolume; expected float64 }{ - {0, -5.0}, - {50, -2.5}, - {100, 0.0}, - } - for _, tt := range tests { - got := tt.user.toInternal() - if math.Abs(got - tt.expected) > 0.01 { - t.Errorf("UserVolume(%d).toInternal() = %f, want %f", tt.user, got, tt.expected) - } +// backend/tageditor/tageditor.go +func NewTagEditor(logger *slog.Logger, db *database.DB) *TagEditor { + return &TagEditor{ + logger: logger.WithGroup("tageditor"), + db: db, } } + +func (te *TagEditor) SetContext(ctx context.Context) { + te.mu.Lock() + defer te.mu.Unlock() + te.ctx = ctx +} ``` -### Mocking Strategy +### Pattern 2: Event-Driven Frontend Sync -**Use real in-memory SQLite, not mocked interfaces.** Reasons: +**What:** Backend emits events; frontend stores subscribe. -1. The `modernc.org/sqlite` driver is pure Go — no CGo, no external deps, fast in-memory mode -2. Mocking the DB interface would require mocking `*sqlcgen.Queries` (dozens of methods) — fragile and doesn't test real query behavior -3. SQLite in-memory is effectively instant — no performance reason to mock -4. Tests that exercise real SQL catch bugs that mock tests miss (FTS5 tokenization, JOIN correctness, migration logic) +**When:** Any state change that the frontend needs to reflect. -**Mock only at narrow interfaces:** -- `TrackLoader` for queue tests (already an interface) -- File system for library scan tests (use `testing/fstest.MapFS` or a temp directory with test audio files) -- Wails runtime can be a no-op `context.Background()` — events fire into the void +**Example:** +```go +// Backend emits +runtime.EventsEmit(te.ctx, events.TagsEdited, map[string]any{ + "filePaths": affectedPaths, +}) -### Risk Assessment - -| Change | Risk | Rationale | -|--------|------|-----------| -| `NewTestDB()` function | **Very Low** | Extracts existing logic, adds `:memory:` path | -| `internal/testdb` helper | **Very Low** | New test-only package | -| Queue tests with mock player | **Low** | Tests new code, doesn't change production code | -| Config tests with TempDir | **Very Low** | Isolated, no production code changes | -| Player pure logic extraction | **Low** | Moving existing code to new functions | - -### Dependencies - -- `NewTestDB()` in database package must be created first — all other test packages depend on it. -- **This is the foundation for safe refactoring** — should be one of the first things built. - ---- - -## Recommended Build Order - -Based on dependency analysis and risk: - -``` -Phase 1: Foundation (no dependencies, enables everything else) -├── 1a. Test architecture (Issue 6) — NewTestDB, testdb helper -├── 1b. Event code generation (Issue 2) — independent, low risk -└── 1c. SetContext mutex fixes (Issue 1) — independent, low risk - -Phase 2: Safety Net (requires Phase 1a) -├── 2a. Queue unit tests — using testdb + mock player -├── 2b. Database/search tests — using testdb -└── 2c. Config tests — using TempDir - -Phase 3: Refactoring (requires Phase 2 tests as safety net) -├── 3a. SQL VIEW + sqlc migration (Issue 5) — search tests verify no regression -├── 3b. Queue incremental persistence (Issue 4) — queue tests verify no regression -└── 3c. Library store lazy loading (Issue 3) — frontend change, lower risk - -Phase 4: Extended Tests -├── 4a. Library scan tests — complex, last because scan code may change during Phase 3 -└── 4b. Player pure logic tests — independent extraction +// Frontend subscribes +EventsOn(Events.TagsEdited, (data: {filePaths: string[]}) => { + // Refresh affected track displays +}); ``` -### Phase Ordering Rationale +### Pattern 3: Config Section Pattern -1. **Tests before refactoring** because the consolidation milestone's entire purpose is safe improvement. Refactoring without tests in a codebase with known concurrency issues is high-risk. +**What:** New config sections follow the `ApplyDefaults()` + `Validate()` pattern. -2. **SetContext fixes (1c) before queue tests (2a)** because the race conditions in SetContext would cause flaky test failures under `-race`. +**When:** Any new user-configurable setting. -3. **SQL VIEW (3a) before queue persistence (3b)** because the VIEW changes the database schema that queue queries depend on. Do schema changes first, then change query patterns. +**Example:** +```go +// backend/shortcuts/config.go +type Config struct { + Bindings []Shortcut `toml:"Bindings"` +} -4. **Frontend lazy loading (3c) last in Phase 3** because it's the lowest-risk change (removing code, not adding it) and is independent of backend refactoring. +func (c *Config) ApplyDefaults() { /* ... */ } +func (c *Config) Validate() error { /* ... */ } +``` + +### Pattern 4: Database Migration for New Tables + +**What:** New tables use `CREATE TABLE IF NOT EXISTS` in schema files + `PRAGMA user_version` migration for any ALTER operations. + +**When:** Adding new persistent data. + +**Example:** Smart playlists table in `backend/database/sql/schemas/smart_playlists.sql` with `CREATE TABLE IF NOT EXISTS`, plus Migration 6 in `database.go` for any column additions. --- ## Anti-Patterns to Avoid -### Anti-Pattern 1: Interface-Heavy Mocking +### Anti-Pattern 1: Frontend-Owned State -**What people do:** Create interfaces for everything (`DatabaseInterface`, `ConfigInterface`) to enable mock-based testing. -**Why it's wrong for this codebase:** SQLite in-memory is as fast as a mock and tests real behavior. Interface proliferation adds complexity without catching real SQL bugs. -**Do this instead:** Use real in-memory SQLite for DB tests. Only create interfaces at natural boundaries (like `TrackLoader`, which already exists). +**What:** Storing authoritative state in frontend stores rather than the backend. -### Anti-Pattern 2: Premature Abstraction of Persistence +**Why bad:** Violates the single-source-of-truth principle. State gets out of sync on refresh, loses persistence. -**What people do:** Build a generic "repository pattern" or ORM-like layer to abstract all SQL. -**Why it's wrong for this codebase:** sqlc already provides type-safe generated code. Adding another abstraction layer on top of sqlc defeats its purpose. -**Do this instead:** Use sqlc queries directly. Use VIEWs for complex JOINs. Hand-craft SQL only for dynamic batch operations where sqlc can't help. +**Instead:** All state changes go through backend. Frontend stores are mirrors. -### Anti-Pattern 3: Global Event Bus Replacement +### Anti-Pattern 2: Direct DB Access from New Packages -**What people do:** Replace Wails events with a custom pub/sub system to enable testing. -**Why it's wrong for this codebase:** The Wails event system is deeply integrated and works well. The real problem (event name parity) is solved by code generation, not by replacing the event system. -**Do this instead:** Use `context.Background()` in tests (events no-op). Add code generation for event names. If event verification is needed later, wrap `runtime.EventsEmit` in a thin injectable function. +**What:** New packages opening their own DB connections or using raw `sql.DB` directly. + +**Why bad:** Violates single-writer constraint. Bypasses sqlc type safety. + +**Instead:** All DB access goes through the shared `*database.DB` instance with sqlc-generated queries. Use `db.BeginTx()` for transactions. Only use raw queries for dynamic SQL (smart playlists), with `// SAFETY:` comments. + +### Anti-Pattern 3: Circular Package Dependencies + +**What:** `tageditor` importing `library` which imports `tageditor`. + +**Why bad:** Go doesn't allow circular imports. + +**Instead:** Use interface-based decoupling (like `TrackLoader` interface) or hook patterns (like `RescanHooks`). The tageditor can accept a `LibraryRefresher` interface rather than importing the library package. + +### Anti-Pattern 4: Blocking the Wails Event Loop + +**What:** Long-running operations in Wails binding methods without goroutines. + +**Why bad:** Freezes the UI. + +**Instead:** Long operations (tag writing, MB API calls, scan) run in goroutines and emit progress events. The binding method returns immediately or returns a "started" acknowledgement. + +### Anti-Pattern 5: Uncontrolled HTTP Requests + +**What:** MusicBrainz API calls without rate limiting. + +**Why bad:** IP gets blocked. MusicBrainz enforces 1 req/sec strictly. + +**Instead:** Single rate-limited HTTP client with `time.Ticker`. Cache all responses. Queue requests. + +--- + +## Build Order (Dependency-Aware) + +### Phase 1: Independent Foundations + +These features have no inter-dependencies and can be built in any order: + +1. **Scan Cancellation** — Smallest change. Modifies existing code minimally. Tests scan pipeline resilience. +2. **Customizable Keyboard Shortcuts** — Config + new package + frontend keydown listener. No data model changes. + +### Phase 2: Data Model Extensions + +These features add new database tables/queries: + +3. **Tag Editing** — New dependency (`bogem/id3v2`), new DB queries, new package. Validates that tag write → DB update → event → frontend refresh pipeline works. +4. **Smart Playlists** — New table, new package, dynamic SQL. Independent of tag editing but benefits from validated DB migration patterns. + +### Phase 3: Complex Backend Changes + +5. **Gapless Playback** — Core audio pipeline modification. Start with gapless, add crossfade later. Most technically risky feature. +6. **MusicBrainz Browser** — First network feature. HTTP client, caching, rate limiting. Independent of other features. + +### Phase 4: Extensibility Foundations + +These are the "foundation" features — functional but not necessarily feature-complete: + +7. **Layout Customization** — Requires all existing components to be working well. Modifies `index.html` structure. +8. **Plugin System** — Must be last — it depends on having a stable API surface from all other features. + +### Rationale for This Order + +- **Scan cancellation first** because it's a quick win that validates context cancellation patterns used throughout. +- **Shortcuts early** because they're simple config + dispatch with no data model changes. +- **Tag editing before smart playlists** because smart playlists query against track metadata that tag editing modifies — testing both together reveals integration issues. +- **Gapless after tag editing** because tag editing validates the "modify player behavior → event → frontend update" pipeline at a simpler level. +- **MusicBrainz after gapless** because it introduces network complexity that's orthogonal to audio — building it later keeps the audio work focused. +- **Layout and plugins last** because they're meta-features that wrap existing features. Building them last means the thing they're wrapping is stable. + +--- + +## Wails Bridge Implications + +### New FEBindings + +Every new backend service added to `FEBindings` in `app.go` generates TypeScript stubs in `frontend/wailsjs/go/`. After adding new bindings: + +```bash +make generate # regenerates Wails bindings + sqlc + events codegen +``` + +### New Events (All Features) + +Estimated new events across all features: + +```go +// Tag editing +TagsEdited = "TagsEdited" +TagEditFailed = "TagEditFailed" + +// Scan cancellation +LibraryScanCancelled = "LibraryScanCancelled" + +// Smart playlists +SmartPlaylistCreated = "SmartPlaylistCreated" +SmartPlaylistUpdated = "SmartPlaylistUpdated" +SmartPlaylistDeleted = "SmartPlaylistDeleted" + +// Shortcuts +ShortcutConfigChanged = "ShortcutConfigChanged" + +// MusicBrainz +MusicBrainzSearchComplete = "MusicBrainzSearchComplete" + +// Layout +LayoutConfigChanged = "LayoutConfigChanged" + +// Gapless/Crossfade +CrossfadeConfigChanged = "CrossfadeConfigChanged" +``` + +All go through the existing AST-based codegen pipeline (`go generate` + pre-commit hook). + +### Database Migrations + +New migration sequence (current version = 5): + +| Migration | Feature | What | +|-----------|---------|------| +| 6 | Smart Playlists | `CREATE TABLE smart_playlists` | +| 7 | MusicBrainz | `CREATE TABLE musicbrainz_cache` | +| 8 | Shortcuts | Config-based (no table needed) | +| 9 | Layout | Config-based (no table needed) | +| 10 | Plugins | `CREATE TABLE plugin_state` (optional, for persistent plugin data) | + +Most features use config (TOML) rather than DB for their settings, keeping migrations minimal. + +--- + +## Scalability Considerations + +| Concern | Current (~1K tracks) | At 10K tracks | At 100K tracks | +|---------|---------------------|---------------|----------------| +| Smart playlist eval | <10ms | <100ms | May need indexing | +| Tag edit (single file) | ~50ms | ~50ms | ~50ms (file-level) | +| Tag edit (batch 100) | — | ~5s (serial writes) | Same | +| FTS5 re-index (tag edit) | ~1ms | ~1ms | ~1ms (single row) | +| MB API browse | Network-bound | Same | Same | +| Layout render | ~5ms | Same | Same | + +The main scalability concern is **smart playlist evaluation** at large library sizes. The `track_metadata` VIEW already has a 5-table JOIN. Adding WHERE clauses for smart playlist rules adds no extra JOINs — the VIEW handles the complexity. SQLite's query planner should handle 100K rows with proper indexes. --- ## Sources -- Codebase analysis: `backend/queue/queue.go`, `backend/queue/persistence.go`, `backend/player/player.go`, `backend/library/library.go`, `backend/config/config.go`, `backend/database/search.go`, `backend/database/database.go`, `backend/events/events.go`, `frontend/src/events.ts`, `frontend/src/store/library-store.ts` — **HIGH confidence** (direct code reading) -- sqlc `sqlc.slice()` for SQLite: `docs.sqlc.dev/en/stable/howto/select.html` — **HIGH confidence** (official documentation, verified) -- sqlc batch operations (`:batchexec` etc.) are PostgreSQL-only: `docs.sqlc.dev/en/stable/reference/query-annotations.html` — **HIGH confidence** (official documentation, verified) -- sqlc VIEW support: sqlc parses `CREATE VIEW` in schema files — **MEDIUM confidence** (documented for PostgreSQL; SQLite support inferred from general DDL handling, needs validation) -- `modernc.org/sqlite` `:memory:` support: standard `database/sql` behavior — **HIGH confidence** (Go stdlib) -- Go `sync.Mutex` patterns: Go stdlib documentation — **HIGH confidence** -- Go `go/ast` for code generation: Go stdlib — **HIGH confidence** +- Codebase analysis: Complete read of all Go packages and TypeScript sources (2026-03-06) +- beep v2.1.1 API: `pkg.go.dev/github.com/gopxl/beep/v2` — Mixer, Seq, Buffer, Ctrl types confirmed +- MusicBrainz API: `musicbrainz.org/doc/MusicBrainz_API` — rate limiting (1 req/sec), JSON format, entity types +- dhowden/tag: Read-only library confirmed from source (`tag.ReadFrom` only, no write methods) +- SQLite WAL mode + single writer: Existing `database.go` configuration confirmed +- Wails v2 binding generation: Existing `app.go` FEBindings pattern confirmed --- -*Architecture research for: YellowJacket consolidation milestone* -*Researched: 2026-02-27* + +*Architecture research: 2026-03-06* diff --git a/.planning/research/FEATURES.md b/.planning/research/FEATURES.md index a04c529..d0b2abc 100644 --- a/.planning/research/FEATURES.md +++ b/.planning/research/FEATURES.md @@ -1,362 +1,550 @@ -# Feature Research: Quality Improvements +# Feature Landscape: v1.1 Features & Extensibility -**Domain:** Go/Wails/Lit desktop music player — consolidation milestone -**Researched:** 2026-02-27 -**Confidence:** HIGH (improvements grounded in codebase analysis + verified patterns) - -## Feature Landscape - -This is a consolidation milestone. "Features" here are quality improvements, not new user-facing functionality. Each improvement addresses a specific concern documented in `.planning/codebase/CONCERNS.md`. +**Domain:** Desktop music player — new capabilities milestone +**Researched:** 2026-03-06 +**Confidence:** HIGH (grounded in codebase analysis, official documentation, established desktop music player patterns) --- -### Table Stakes (Must Fix — Codebase Is Unreliable Without These) +## Overview -These are correctness and reliability issues. Leaving them unfixed means the codebase has known race conditions, swallowed errors, and untested critical paths. - -| Improvement | Why Required | Complexity | Concern Ref | -|-------------|-------------|------------|-------------| -| **Fix SetContext data races in Queue, Library, Playlist** | `q.ctx`, `l.ctx`, `s.ctx` are written without locks but read under locks. This is a textbook data race detectable by `-race`. Even if startup ordering makes it safe today, any refactoring that changes init order silently introduces corruption. | LOW | Concurrency Concerns | -| **Fix package-level `startupErr` variable** | Mutable package-level variable shared between `OnStartup` and `OnDomReady`. Not thread-safe, untestable. Move to `YellowJacketApp` struct field. | LOW | Tech Debt | -| **Fix config file permissions (0o666 → 0o644)** | Writing world-writable config files is a security defect. One-line fix. | LOW | Error Handling Gaps | -| **Fix swallowed errors in MPRIS lifecycle callbacks** | `_ =` on `Pause()` and `Seek()` errors from OS media controls. Invisible failures. At minimum log; ideally emit frontend notification. | LOW | Error Handling Gaps | -| **Fix silently swallowed artist credit link error** | `_, _ = CreateArtistCreditArtist(...)` discards non-duplicate errors. Check error, ignore only UNIQUE constraint violations. | LOW | Error Handling Gaps | -| **Separate scan warnings from fatal errors** | `Scan()` returns `errors.Join()` of all errors. Callers cannot distinguish "scan completed with 3 file warnings" from "scan completely failed". Return warnings in metrics, fatal errors as the error return. | MEDIUM | Error Handling Gaps | -| **Unit tests for queue operations** | Queue is central to playback — SetQueue, navigation, shuffle, repeat, persistence — all untested. Bugs here cause tracks to skip, repeat wrong, or lose queue on restart. | HIGH | Test Coverage Gaps | -| **Unit tests for library scan logic** | Metadata processing, entity cache, orphan cleanup — all untested. Bugs silently drop tracks or create duplicates. | HIGH | Test Coverage Gaps | -| **Unit tests for database layer (FTS5, migrations)** | FTS5 edge cases (special chars, empty queries) and migration failures are completely untested. | MEDIUM | Test Coverage Gaps | -| **Unit tests for config (load/save roundtrip)** | Config corruption or silent settings loss on upgrade has no safety net. | MEDIUM | Test Coverage Gaps | - -#### Concurrency Fix Details - -**Pattern:** For `SetContext` race conditions, the fix is uniform across Queue, Library, and Playlist: - -```go -// BEFORE (Queue — race condition): -func (q *Queue) SetContext(ctx context.Context) { - q.ctx = ctx // no lock, but q.ctx read under q.mu elsewhere -} - -// AFTER (correct): -func (q *Queue) SetContext(ctx context.Context) { - q.mu.Lock() - defer q.mu.Unlock() - q.ctx = ctx -} -``` - -Player already does this correctly (locks around `p.ctx = ctx` in `SetContext`). Apply the same pattern to Queue, Library, and Playlist. For Library and Playlist which don't currently have a mutex, add one — or document the "set during startup only, before any concurrent access" contract with a comment and `// SAFETY:` annotation. - -**Recommendation:** Add a `sync.Mutex` to Library and Playlist. The cost is negligible, and it eliminates the `-race` detector finding permanently. Documenting "safe because startup ordering" is fragile — the next developer (or future-you) may change init order. *Confidence: HIGH — standard Go concurrency practice.* - -#### Testing Strategy Details - -**In-memory SQLite for DB-dependent tests:** Use `sql.Open("sqlite", ":memory:")` with the `modernc.org/sqlite` driver (already in deps). Apply the same schema migrations used in production. This gives: -- Fast test execution (no disk I/O) -- Clean state per test (new DB per test function) -- Identical query behavior to production - -**Pattern for queue/library tests:** -```go -func setupTestDB(t *testing.T) *database.DB { - t.Helper() - db, err := database.NewTestDB(t) // in-memory, migrations applied - require.NoError(t, err) - return db -} - -func TestSetQueueAndNavigate(t *testing.T) { - db := setupTestDB(t) - q := queue.NewQueue(slog.Default(), db) - // No SetContext needed — test without Wails runtime - // Test pure queue logic without event emission -} -``` - -**Extract testable pure logic from Player:** Volume math (`UserVolume` → `Volume` conversion), state serialization, and format detection can be tested without audio hardware. Create `volume_test.go` with pure function tests. *Confidence: HIGH — standard Go testing pattern.* - -**Event-driven testing approach:** For packages that emit events, provide a test double or capture mechanism. Options: -1. Accept an `EventEmitter` interface (allows mock in tests) -2. Make event emission optional when `ctx == nil` (already partially the case — `emit` methods check for nil context) -3. Test state mutations independent of event emission - -**Recommendation:** Option 2 is already partially implemented. Lean into it: test queue/library state mutations without Wails context, verify state is correct, don't test event emission in unit tests. *Confidence: HIGH.* +This research covers 8 feature areas for YellowJacket v1.1: tag editing, scan cancellation, smart playlists, customizable keyboard shortcuts, gapless playback + crossfade, MusicBrainz browser, layout customization, and plugin system. Each is categorized as table stakes, differentiator, or anti-feature relative to the desktop music player domain. --- -### Differentiators (Raises Quality Significantly) +## 1. Tag Editing -These improvements go beyond "not broken" to "genuinely well-engineered." They improve performance, maintainability, and user experience noticeably. +### Table Stakes -| Improvement | Value Proposition | Complexity | Concern Ref | -|-------------|-------------------|------------|-------------| -| **Eliminate duplicated FTS5 JOIN query pattern** | Same 5-table JOIN repeated 5+ times across search functions. Schema changes require updating all copies. Extract into shared constant or consolidate into fewer sqlc queries. | MEDIUM | Code Quality | -| **Migrate raw SQL in queue persistence to sqlc** | `lookupChunk` and `insertTrackBatch` use `fmt.Sprintf` for batch operations. Use `sqlc.slice()` for lookups. Batch inserts can remain hand-crafted but documented. | MEDIUM | Code Quality | -| **Optimize library store — lazy loading instead of eager fetch** | `eagerFetch()` loads all tracks, albums, artists, genres simultaneously on startup. For 50k+ tracks, this is tens of MB of JS objects loaded before user sees anything. Load only the active view's data. | HIGH | Performance | -| **Optimize queue persistence — incremental updates** | Every add/remove/move does DELETE ALL + INSERT ALL. For a 5000-track queue, every single mutation rewrites the entire table. Use INSERT/DELETE for individual operations; reserve full rewrite for SetQueue. | MEDIUM | Performance | -| **Fix SetQueue Phase 2 redundant lookups** | Phase 2 re-fetches metadata for ALL file paths including those already resolved in Phase 1. Pass Phase 1 results to Phase 2, only lookup remaining paths. | LOW | Performance | -| **Extract testable player logic** | Volume conversion, state serialization, format detection — all testable without audio hardware. Currently locked inside Player struct behind hardware dependency. | LOW | Test Coverage | -| **Event name parity validation** | Event names must match exactly between Go and TypeScript. No compile-time or runtime verification. Add a build-time check (code generation or test). | LOW | Fragile Areas | -| **Polish UI transitions and visual consistency** | CSS transitions for panel open/close, list item hover states, loading skeletons. Makes the app feel responsive and intentional. | MEDIUM | UX | -| **Improve frontend rendering for large libraries** | Even with `lit-virtualizer`, store updates trigger re-renders. Optimize with `repeat()` directive keyed by stable IDs, memoized render functions, and avoiding full-array replacement on updates. | MEDIUM | Performance | +| Feature | Why Expected | Complexity | Dependencies | +|---------|--------------|------------|--------------| +| Edit title, artist, album, genre, year, track number | Every music manager (MusicBee, foobar2000, Clementine, Strawberry) supports this. Users expect to correct metadata without leaving the app. | MEDIUM | Existing metadata extraction pipeline, new tag writing libraries | +| Edit single track | Right-click → edit properties is the universal pattern | LOW | Tag writing backend | +| Batch edit multiple tracks | Select multiple → edit shared fields (e.g., set all to same album). This is the primary workflow for fixing album imports. | MEDIUM | Single-track editing must work first | +| Write changes to actual audio files | Tags must persist to the file on disk, not just the DB. Users expect changes to survive re-imports and transfers to other players. | MEDIUM | Tag writing libraries (format-specific) | +| Update DB after tag write | After writing tags to file, the DB must reflect the new metadata without requiring a full rescan. | LOW | Existing DB update queries | +| Cover art assignment | Set/replace embedded cover art from an image file | MEDIUM | Image handling + tag writing | -#### FTS5 Query Consolidation Details +### Differentiators -**Current state:** The same JOIN pattern appears in: -1. `SearchFTS()` — 5 columns -2. `SearchFTSByFilename()` — 5 columns (same query, different WHERE) -3. `SearchFTSTracks()` — 16 columns (extended version) -4. `RebuildSearchIndex()` — 5 columns (INSERT INTO ... SELECT) -5. `migration2BasenameAndFTS()` — same pattern in migration +| Feature | Value Proposition | Complexity | Dependencies | +|---------|-------------------|------------|--------------| +| Undo/redo for tag edits | Safety net — rare in music players, very valued when present | HIGH | Requires edit history tracking | +| Auto-capitalize/clean tag values | Consistent library appearance with minimal effort | LOW | String utilities | +| Filename-to-tag inference | Parse "Artist - Title.mp3" patterns to pre-fill fields | MEDIUM | Regex/pattern engine | +| Tag-to-filename rename | Rename files based on tag template (e.g., "%artist% - %title%.%ext%") | HIGH | File system operations, template engine | -**Recommended approach:** Create a SQL view for the common JOIN: +### Anti-Features -```sql -CREATE VIEW IF NOT EXISTS track_metadata_view AS -SELECT - af.id AS audio_file_id, - af.file_path, - af.length_milliseconds, - COALESCE(r.name, '') AS title, - COALESCE(ac.text, '') AS artist, - COALESCE(rg.name, '') AS album, - r.track_number, - r.disc_number, - -- ... other fields -FROM audio_files af -LEFT JOIN recordings r ON af.recording_id = r.id -LEFT JOIN artist_credit ac ON r.artist_credit_id = ac.id -LEFT JOIN ( - SELECT recording_id, MIN(release_group_id) AS release_group_id - FROM release_group_recordings - GROUP BY recording_id -) rgr ON r.id = rgr.recording_id -LEFT JOIN release_groups rg ON rgr.release_group_id = rg.id; +| Anti-Feature | Why Avoid | What to Do Instead | +|--------------|-----------|-------------------| +| Auto-tag from online DB in tag editor | Conflates two features — tag editing and metadata lookup. MusicBrainz browser is the separate feature for this. | Keep tag editing purely manual; MusicBrainz browser is the lookup tool | +| Destructive batch operations without confirmation | Mass edits can corrupt a library. | Always show preview/confirmation dialog for batch edits | +| Writing tags during playback of that file | File locking conflicts on Windows; potential corruption on any OS | Queue the write for after playback stops, or copy-on-write | + +### Implementation Notes + +**Tag writing requires format-specific libraries (the existing `dhowden/tag` is read-only):** + +- **MP3 (ID3v2):** `github.com/bogem/id3v2/v2` — mature, pure Go, supports ID3v2.3/2.4 read+write, handles text frames, pictures, comments. Confirmed: `tag.Open()` → `tag.SetArtist()` → `tag.Save()` pattern. v2.1.4 is current. +- **FLAC (Vorbis Comments):** `github.com/go-flac/go-flac` + `github.com/go-flac/flacvorbis` — parse FLAC file, modify vorbis comment metadata blocks, save back. Confirmed: `flac.ParseFile()` → modify `Meta` slice → `f.Save()`. v1.0.0/v0.2.0 current (v2 exists). +- **OGG Vorbis:** No mature pure-Go write library exists. Options: (a) skip OGG tag writing initially, (b) use `go-flac/flacvorbis`-style approach with raw vorbis comment manipulation if a library surfaces, or (c) shell out to `vorbiscomment` CLI tool. +- **WAV:** WAV metadata (INFO chunks, ID3 headers) is rarely edited. Skip for v1.1. + +**Critical constraint:** The existing `dhowden/tag` library is read-only. Tag writing is a completely separate code path requiring new dependencies. Tag reading continues through `dhowden/tag`; writing uses format-specific libraries. + +**DB sync pattern:** After writing tags to file, update the specific DB rows rather than triggering a full rescan. Extract the new metadata from the written file (or trust the values just written), update the `recordings`, `artists`, `release_groups`, and `audio_files` tables, then emit a `TrackMetadataChanged` event to sync the frontend. + +--- + +## 2. Scan Cancellation + +### Table Stakes + +| Feature | Why Expected | Complexity | Dependencies | +|---------|--------------|------------|--------------| +| Cancel button during scan | Large libraries take minutes to scan. Users expect to be able to stop a scan in progress. Every file manager and media player with scanning provides this. | LOW | Existing scan pipeline with `context.Context` | +| Graceful stop (don't corrupt DB) | Cancellation must not leave the DB in an inconsistent state. Complete in-flight transactions, skip remaining files. | LOW | Existing transaction batching | +| Scan progress reporting | Users need to see what's happening — "Processing 340/2000 files" — to decide whether to wait or cancel. | LOW | Existing `ScanProgress` event (already partially implemented) | + +### Differentiators + +| Feature | Value Proposition | Complexity | Dependencies | +|---------|-------------------|------------|--------------| +| Pause and resume scan | Stop temporarily, resume later without re-scanning already-processed files | HIGH | Would need scan state persistence | +| Background scan with low priority | Scan without impacting playback or UI responsiveness | LOW | Already partially handled by worker pool concurrency tuning | + +### Anti-Features + +| Anti-Feature | Why Avoid | What to Do Instead | +|--------------|-----------|-------------------| +| Immediate hard kill (kill goroutines) | Data corruption risk — partial writes, broken entity caches | Use context cancellation for cooperative shutdown | +| Auto-cancel on any error | Users want the scan to continue past individual file failures | Continue scanning, accumulate warnings (already the pattern) | + +### Implementation Notes + +**The existing scan pipeline already uses `context.Context` — `l.ctx` is available throughout the scan.** The implementation pattern is straightforward: + +1. Create a cancellable context: `scanCtx, cancelScan := context.WithCancel(l.ctx)` +2. Store `cancelScan` so the frontend can trigger it via a Wails binding (e.g., `Library.CancelScan()`) +3. Check `scanCtx.Done()` in the filesystem walk loop, the worker pool dispatch, and the DB writer +4. On cancellation, the `errgroup` returns `context.Canceled`, which is caught and treated as a clean stop +5. Emit `LibraryScanCancelled` event (distinct from `LibraryScanComplete`) + +**Key insight:** The existing scan already uses `errgroup` which respects context cancellation. The DB writer goroutine processes whatever is in its batch channel, so in-flight batches complete cleanly. The only new code needed is: (a) storing/exposing the cancel function, (b) checking context in the walk loop, (c) a new event for cancellation. + +**Complexity is LOW** because the architecture already supports this pattern. The scan pipeline's multi-phase design means cancellation at any phase is naturally bounded. + +--- + +## 3. Smart Playlists + +### Table Stakes + +| Feature | Why Expected | Complexity | Dependencies | +|---------|--------------|------------|--------------| +| Filter by genre | "All Jazz tracks" — the most basic smart playlist rule | LOW | Existing genre data in DB | +| Filter by year/year range | "Tracks from 1990-1999" | LOW | Existing year field in DB | +| Filter by artist | "All tracks by Artist X" | LOW | Existing artist data | +| Combine multiple rules (AND) | "Jazz tracks from the 1990s" — users expect to stack filters | MEDIUM | Rule evaluation engine | +| Auto-update when library changes | Smart playlists should refresh when tracks are added/removed. This is the defining feature vs. manual playlists. | MEDIUM | Event subscription to library changes | +| Name and save smart playlists | Persist rule definitions, show in sidebar alongside regular playlists | LOW | New DB table for rule definitions | + +### Differentiators + +| Feature | Value Proposition | Complexity | Dependencies | +|---------|-------------------|------------|--------------| +| Filter by play count | "Most played" / "Never played" — requires play count tracking (not currently implemented) | MEDIUM | New `play_count` column or table | +| Filter by date added | "Recently added" — very popular smart playlist | LOW | Existing file modification time or new `added_at` column | +| Filter by rating | Requires rating system (not currently implemented) | MEDIUM | New rating feature | +| OR logic and nested groups | "(Genre=Jazz OR Genre=Blues) AND Year>1980" — powerful but complex UI | HIGH | Recursive rule evaluation, complex UI builder | +| Random/limit results | "Random 50 Jazz tracks" — playlist-as-radio | LOW | SQL `ORDER BY RANDOM() LIMIT N` | +| Sort order in rules | "Newest first" / "Alphabetical by artist" | LOW | SQL `ORDER BY` clause | + +### Anti-Features + +| Anti-Feature | Why Avoid | What to Do Instead | +|--------------|-----------|-------------------| +| Full SQL WHERE clause as input | Exposes DB internals, injection risk, terrible UX | Structured rule builder with defined fields and operators | +| Complex nested boolean logic in v1 | Overwhelms users, complex UI, rarely used | Start with flat AND rules; add OR/nesting later if demanded | +| Real-time updating during playback | Unnecessary overhead — smart playlists don't need sub-second freshness | Refresh on library scan completion and on explicit refresh | + +### Implementation Notes + +**Rule model — keep it simple for v1:** + +``` +SmartPlaylistRule { + Field: "genre" | "year" | "artist" | "album" | "title" | "date_added" + Operator: "equals" | "not_equals" | "contains" | "greater_than" | "less_than" | "between" + Value: string (or string pair for "between") +} + +SmartPlaylist { + ID: int64 + Name: string + Rules: []SmartPlaylistRule // all ANDed together + SortField: string (optional) + SortOrder: "asc" | "desc" + Limit: int (0 = unlimited) +} ``` -Then search queries become `SELECT ... FROM search_index si JOIN track_metadata_view tmv ON tmv.audio_file_id = si.rowid WHERE search_index MATCH ?`. Single source of truth for the JOIN pattern. +**Storage:** New `smart_playlists` table (id, name, rules_json, sort_field, sort_order, limit_count) with rules stored as JSON in a TEXT column. This avoids a complex relational schema for rules and is trivially extensible. -**Alternative:** Extract the JOIN clause as a Go string constant and compose queries from it. Less elegant but simpler to implement. +**Query generation:** Each rule maps to a SQL WHERE clause fragment. Rules are joined with AND. The existing `track_metadata` VIEW provides all the needed columns for filtering. Generated SQL uses parameterized queries (NOT string concatenation) to prevent injection. -**Recommendation:** Use the SQL view approach. SQLite views are essentially macros — no performance penalty. They can be referenced in sqlc queries. Add the view to the schema, then rewrite search queries against it. *Confidence: MEDIUM — SQLite views in sqlc need verification during implementation. The concept is sound, but sqlc's handling of views with FTS5 virtual tables may have edge cases.* +**Refresh strategy:** Smart playlists evaluate lazily — results are computed on access and cached. Cache is invalidated on `LibraryScanComplete` events. This avoids expensive re-evaluation on every library change. -#### Queue Persistence Optimization Details +**Depends on:** Existing `track_metadata` VIEW, playlist sidebar UI, event system. -**Current pattern:** -``` -Every mutation → commitMutation() → persistTracks() → DELETE ALL + batch INSERT ALL -``` +--- -**Improved pattern:** -``` -AddTrack → INSERT single row + shift positions -RemoveTrack → DELETE single row + shift positions -MoveTrack → UPDATE positions for affected range -SetQueue / RestoreState → DELETE ALL + batch INSERT ALL (keep current) -``` +## 4. Customizable Keyboard Shortcuts -The sqlc queries `InsertQueueTrack`, `RemoveQueueTrack`, `ShiftQueuePositionsDown`, `ShiftQueuePositionsUp` already exist but aren't used by `commitMutation()`. Wire them up for single-track operations. +### Table Stakes -*Confidence: HIGH — the individual queries already exist in sqlc.* +| Feature | Why Expected | Complexity | Dependencies | +|---------|--------------|------------|--------------| +| Play/pause hotkey | Space bar is universal; must work | LOW | Existing player controls | +| Next/previous track | Arrow keys or media key equivalents | LOW | Existing queue navigation | +| Volume up/down | Standard audio app functionality | LOW | Existing volume control | +| Mute toggle | Expected in any audio application | LOW | Existing mute functionality | +| Search focus | Ctrl+F or / to focus search — standard in any list-heavy app | LOW | Existing search bar | +| Default keybindings that work out of box | Users shouldn't have to configure anything to get basic shortcuts | LOW | Hardcoded defaults with override capability | -#### Library Store Lazy Loading Details +### Differentiators -**Current:** Constructor calls `eagerFetch()` → 4 parallel Wails binding calls → 4 full table scans with JOINs → all data in JS memory. +| Feature | Value Proposition | Complexity | Dependencies | +|---------|-------------------|------------|--------------| +| Full customization UI | Visual keybinding editor with conflict detection | MEDIUM | Settings page extension | +| Import/export keybindings | Share/backup custom configs | LOW | TOML serialization (already used for config) | +| Scoped shortcuts (global vs. component-specific) | Different bindings when focus is in search vs. track list | MEDIUM | Focus tracking | +| "When focused" context awareness | Arrows navigate track list when it's focused, but control volume when player is focused | MEDIUM | Component focus management | + +### Anti-Features + +| Anti-Feature | Why Avoid | What to Do Instead | +|--------------|-----------|-------------------| +| Global OS-level hotkeys (outside app window) | Platform-specific, conflicts with OS shortcuts, security concerns on Wayland | App-scoped shortcuts only; MPRIS2 handles media keys | +| Vim-mode or complex modal keybindings | Niche appeal, confusing for 99% of users | Simple single/modifier key combos (Ctrl+X, Shift+X) | +| Shortcut for every possible action | Overwhelming configuration UI | Cover the 10-15 most common actions; rest accessible via menus | + +### Implementation Notes + +**Architecture — event-driven, backend-aware:** + +The shortcut system has two layers: +1. **Frontend key listener:** Captures keyboard events at the document level, maps keystrokes to action names using a binding table +2. **Action dispatch:** Frontend calls the appropriate Wails binding or emits a frontend event for UI-only actions + +**Binding table structure:** -**Improved pattern:** ```typescript -class LibraryStore { - // Load on first access, not constructor - async getTracks(): Promise { - if (this.tracks !== null) return this.tracks; - // ... existing lazy logic (already implemented!) - } - - // Remove eagerFetch() from constructor - constructor() { - EventsOn(Events.LibraryScanComplete, () => this.invalidate()); - this.loadCoverSize(); - // Don't call eagerFetch() — let components trigger loading - } +interface KeyBinding { + action: string; // "play_pause", "next_track", "volume_up", etc. + key: string; // "Space", "ArrowRight", etc. (KeyboardEvent.key) + modifiers: string[]; // ["ctrl"], ["shift"], ["ctrl", "shift"], [] + scope?: string; // "global" | "tracklist" | "queue" (optional, default "global") } ``` -The store *already has* lazy loading logic in `getTracks()`, `getAlbums()`, etc. The only change needed is removing the `eagerFetch()` call from the constructor and from `invalidate()`. Components already call the async getters. The eager fetch is redundant. +**Storage:** Add `[Shortcuts]` section to TOML config. Default bindings are hardcoded; user overrides merge on top. Config change emits `ShortcutConfigChanged` event. -**For even larger libraries (100k+):** Consider pagination. Backend already returns full result sets — add `LIMIT/OFFSET` or cursor-based pagination to the sqlc queries. Frontend virtualizer already handles rendering — it just needs a data provider that fetches pages instead of the full list. +**Conflict detection:** When user changes a binding, check for conflicts within the same scope. Show warning if two actions share the same keystroke. -*Confidence: HIGH — the lazy loading infrastructure already exists.* +**Default bindings (the 12 essentials):** -#### Frontend Performance Details +| Action | Default Key | Scope | +|--------|-------------|-------| +| Play/Pause | Space | global | +| Stop | . (period) | global | +| Next Track | Ctrl+Right | global | +| Previous Track | Ctrl+Left | global | +| Volume Up | Ctrl+Up | global | +| Volume Down | Ctrl+Down | global | +| Mute | M | global | +| Search Focus | Ctrl+F | global | +| Toggle Queue | Q | global | +| Toggle Shuffle | S | global | +| Toggle Repeat | R | global | +| Select All (track list) | Ctrl+A | tracklist | -**Already in place:** `@lit-labs/virtualizer` with `flow` layout for track-list and `grid` layout for cover-grid. This handles DOM virtualization. - -**Additional optimizations:** -1. **Use `repeat()` with stable keys for virtualized lists.** Lit's `repeat` directive reorders DOM nodes instead of recreating them when list order changes. Use `track.filePath` as key (unique, stable). -2. **Avoid full-array replacement in store updates.** When a scan completes, `invalidate()` sets `tracks = null` forcing a full refetch. Instead, diff the new data against cached data and apply deltas. For scan completion, a full invalidation is appropriate, but for queue mutations, use the delta protocol already in place (`applyTracksDelta`). -3. **Debounce store notifications.** When multiple store properties update in rapid succession (e.g., during scan), batch notifications using `queueMicrotask()` instead of notifying per-property. - -*Confidence: MEDIUM — `repeat()` performance gains depend on the update patterns. For initially sorted lists that rarely reorder, `map()` is equally fast. For the cover-grid with resize/reflow, `repeat()` is clearly beneficial.* +**Key insight:** Keyboard shortcuts must NOT interfere with text input. When a text input or textarea has focus, the shortcut system must be disabled (except for Escape to blur). This is the #1 pitfall in keyboard shortcut implementations. --- -### Anti-Features (Things to Deliberately NOT Do During Refactoring) +## 5. Gapless Playback + Crossfade -| Anti-Pattern | Why Tempting | Why Problematic | What to Do Instead | -|-------------|-------------|-----------------|-------------------| -| **Splitting large files purely for line count** | `playlist.go` (1778 lines) and `library.go` (1328 lines) feel large. Some components exceed 2000 lines. | The project explicitly decided against cosmetic splitting (PROJECT.md: "No cosmetic file splitting"). Splitting for its own sake creates navigation overhead and can break logical grouping. | Extract only when it enables reuse (e.g., shared controllers) or fixes a real problem (e.g., testing). | -| **Adding a full ORM or query builder** | Raw SQL in `lookupChunk`/`insertTrackBatch` feels inconsistent with sqlc-generated code. | An ORM would fight the existing sqlc architecture. A query builder adds a dependency for 2-3 queries. The hand-crafted SQL is safe (parameterized) and performant. | Document the hand-crafted queries with `// SAFETY:` comments explaining why they're not in sqlc. Use `sqlc.slice()` where it fits. Accept that batch INSERT with dynamic row count is a legitimate sqlc gap for SQLite. | -| **Rewriting the event system** | Event names are fragile strings that must match between Go and TypeScript. A typed event system would be safer. | The current system works. A rewrite touches every component in both frontend and backend. The risk-to-reward ratio is terrible for a consolidation milestone. | Add a build-time parity check (a test or codegen script that compares event constants). Fix the symptom (fragility) not the architecture. | -| **Adding frontend unit tests for all components** | No frontend tests exist. The temptation is to add comprehensive Lit component testing. | Large Lit components (1400-2600 lines) are expensive to test in isolation. Testing requires JSDOM or a browser harness, Shadow DOM handling, and Wails binding mocks. The backend is the source of truth — frontend bugs are visual, not data-corruption. | Test frontend-only logic (search ranking, column sorting, selection controller) as pure function tests if extracted. Defer full component testing to a future milestone. | -| **Making all queue mutations atomic/transactional from Go to frontend** | The delta protocol between queue store and backend could diverge. Adding sequence numbers or full-state hashes seems robust. | The existing `QueueChanged` event already acts as periodic full-state correction. Adding a sequence protocol adds complexity to every mutation path for a problem that manifests as a temporary visual glitch, self-correcting on the next full emit. | Keep the existing delta + periodic full-state pattern. If divergence becomes a real problem (not theoretical), add a generation counter then. | -| **Over-engineering error types** | The project uses sentinel errors and `fmt.Errorf("%w")`. Defining custom error types with fields (e.g., `ScanError{File, Phase, Cause}`) seems more structured. | Custom error types add boilerplate for minimal benefit in a desktop app. The structured logging already captures context via slog key-value pairs. Error types shine in API servers where callers branch on error details — not here. | Keep sentinel errors for `errors.Is()` checks. Keep `fmt.Errorf("%w")` for wrapping with context. Use `errors.Join()` for accumulation. Separate warnings from fatal errors in scan results via the return signature, not error types. | -| **Adding connection pooling or health checks for SQLite** | PROJECT.md mentions "No Database Connection Pooling/Health Check" in missing features. | This is a desktop app with a local SQLite file and `SetMaxOpenConns(1)`. Connection pooling is meaningless. Health checks add complexity for a failure mode (corrupt SQLite file) that's better handled by "show error dialog, suggest DB reset." | Leave as-is. This was correctly scoped as out-of-scope in PROJECT.md. | -| **Wrapping the entire test suite in Docker for CI** | Integration tests require audio hardware. Docker could theoretically provide a virtual audio device. | Massive CI complexity for marginal benefit. The goal is to make unit tests work without hardware, not to make integration tests work in CI. | Extract testable pure logic. Run unit tests in CI. Keep integration tests as manual/local-only with `YELLOWJACKET_INTEGRATION=1`. | +### Table Stakes + +| Feature | Why Expected | Complexity | Dependencies | +|---------|--------------|------------|--------------| +| Gapless playback (no silence between tracks) | Expected by any serious music listener. Albums are meant to flow. Strawberry, foobar2000, Deadbeef, Audacious all support this. | HIGH | Fundamental change to audio pipeline | +| Crossfade setting (on/off, duration) | Standard feature in every modern music player. Even basic mobile players have this. | MEDIUM | Gapless infrastructure + mixer | +| Crossfade duration control | Users expect 1-10 second configurable fade | LOW | UI slider + config storage | +| Gapless without crossfade (default) | Pure gapless (no overlap) should be the default. Crossfade is opt-in. | HIGH | Pre-decode/buffer next track | + +### Differentiators + +| Feature | Value Proposition | Complexity | Dependencies | +|---------|-------------------|------------|--------------| +| Per-album gapless (auto-detect live albums) | Disable crossfade within albums, enable between albums | MEDIUM | Album boundary detection in queue | +| ReplayGain normalization | Consistent volume across tracks from different sources | HIGH | ReplayGain tag parsing + volume adjustment | +| Fade-in on play, fade-out on pause | Smoother start/stop experience | LOW | Volume envelope on play/pause | + +### Anti-Features + +| Anti-Feature | Why Avoid | What to Do Instead | +|--------------|-----------|-------------------| +| DSP effects chain (equalizer, reverb, etc.) | Scope explosion — not part of gapless/crossfade | Defer to plugin system if ever needed | +| Crossfade for all transitions (including manual skip) | Crossfade on skip feels sluggish | Only crossfade on auto-advance; manual skip is instant | +| Pre-loading entire tracks into memory | Memory explosion with FLAC files (50-100MB per track) | Buffer only the crossfade overlap region (last/first N seconds) | + +### Implementation Notes + +**This is the highest-complexity feature in v1.1.** The current audio pipeline plays one track at a time with a single streamer chain. Gapless playback requires pre-decoding the next track and seamlessly transitioning. + +**Current pipeline:** `file → decode → resample → BufferedStreamer → Ctrl → Volume → Speaker` + +**Gapless pipeline (conceptual):** +1. When current track is N seconds from ending, pre-load next track's decoder + resampler +2. For pure gapless: use `beep.Seq()` to chain current and next streamer — but Seq doesn't support the pre-decode timing +3. For crossfade: use `beep.Mixer` to overlap the fade-out of current with fade-in of next + +**beep library support:** +- `beep.Mixer` — adds/mixes multiple streamers. This is the foundation for crossfade. +- `beep.Seq()` — sequences streamers end-to-end. Foundation for gapless without crossfade. +- `effects.Volume` — volume control already used; can create fade curves by adjusting volume over time. +- `beep.Take()` — extract N samples from a streamer. Useful for defining crossfade regions. + +**Architecture change required:** +- The `Player` must manage TWO streamer chains simultaneously during crossfade +- A `TransitionManager` or equivalent coordinates pre-loading the next track +- The `playbackFinishedHandler` (callback from beep when track ends) must trigger next-track pre-loading rather than waiting for the callback +- The `Queue` must expose a "peek next" capability (already has `tracks` and `currentIndex`) + +**Crossfade implementation sketch:** +``` +[Track A ~~~~~~~~ fade-out] + [fade-in ~~~~~~~~ Track B] + |-- overlap (N seconds) --| +``` +- Track A's volume ramps from 1.0 → 0.0 over N seconds +- Track B's volume ramps from 0.0 → 1.0 over N seconds +- Both feed into a `beep.Mixer` during the overlap period +- After overlap, Track A is closed, Track B continues alone + +**Config addition:** `[Playback]` section with `GaplessEnabled` (bool, default true), `CrossfadeEnabled` (bool, default false), `CrossfadeDurationMs` (int, default 3000, range 500-10000). + +**Critical constraint:** The beep `speaker.Play()` can only be called once; the speaker's mixer is the root. All track management must happen within the streamer chain that the speaker is already playing. This means using a persistent `beep.Mixer` as the root streamer, adding/removing track streamers from it. + +--- + +## 6. MusicBrainz Browser + +### Table Stakes + +| Feature | Why Expected | Complexity | Dependencies | +|---------|--------------|------------|--------------| +| Search artists by name | The entry point — user types artist name, gets results | MEDIUM | MusicBrainz API integration, HTTP client | +| View artist discography (release groups) | Browse albums/EPs/singles by an artist | MEDIUM | API browse: release-groups by artist | +| View album track listing | See what tracks are on a release | MEDIUM | API lookup: release with recordings | +| View album editions (releases within a release group) | Different pressings, reissues, deluxe editions | MEDIUM | API browse: releases by release-group | +| Rate limiting compliance | MusicBrainz requires max 1 request/second with meaningful User-Agent | LOW | HTTP rate limiter, User-Agent header | +| Offline-safe (read-only, no writes) | Read-only browsing — no MusicBrainz account needed | LOW | No authentication required for reads | + +### Differentiators + +| Feature | Value Proposition | Complexity | Dependencies | +|---------|-------------------|------------|--------------| +| Link local tracks to MusicBrainz recordings | Associate library tracks with MBIDs for definitive identity | HIGH | Matching algorithm, DB schema changes | +| Show cover art from Cover Art Archive | Display album art from MusicBrainz's linked image archive | MEDIUM | coverartarchive.org API | +| Cache API responses locally | Avoid re-fetching on every browse session | MEDIUM | SQLite cache table with TTL | +| Search recordings | Find specific songs across all releases | LOW | MusicBrainz recording search API | + +### Anti-Features + +| Anti-Feature | Why Avoid | What to Do Instead | +|--------------|-----------|-------------------| +| Auto-tag from MusicBrainz | This is Picard's domain — extremely complex matching logic | Read-only browsing only. Users can manually apply info from browse to tag editor. | +| Write data to MusicBrainz | Requires OAuth, community guidelines compliance, edit approval | Strictly read-only | +| Download/stream from MusicBrainz | MusicBrainz is a metadata database, not a music source | Display metadata only | +| Background MusicBrainz scanning of entire library | Rate limiting makes this impractical (1 req/sec = 3600 tracks/hour max) | On-demand browsing only | + +### Implementation Notes + +**MusicBrainz API:** REST API at `https://musicbrainz.org/ws/2/`. JSON format via `fmt=json` parameter. No API key required, but must set meaningful User-Agent header: `YellowJacket/ (contact-url-or-email)`. + +**Rate limiting:** Strict 1 request/second. Implement with a `time.Ticker`-based rate limiter in the Go backend. All API calls go through a single rate-limited HTTP client. + +**Go libraries available:** +- `github.com/michiwend/gomusicbrainz` — Go client, but may be outdated +- `go.uploadedlobster.com/musicbrainzws2` — another Go client on SourceHut +- **Recommended: Build a thin HTTP client** — the API is simple REST/JSON. A custom client with rate limiting, User-Agent, and JSON parsing is ~200 lines and avoids third-party dependency risk. + +**API patterns needed for read-only browsing:** +1. **Search artist:** `GET /ws/2/artist?query=&fmt=json&limit=25` +2. **Artist discography:** `GET /ws/2/release-group?artist=&fmt=json&limit=100&inc=artist-credits` +3. **Release group releases:** `GET /ws/2/release?release-group=&fmt=json&inc=media+recordings` +4. **Release track listing:** `GET /ws/2/release/?fmt=json&inc=recordings+media+artist-credits` + +**Frontend architecture:** New view (`musicbrainz-browser` component) accessible from sidebar. Search bar, results list, detail panels for artist/album/release. Navigation is drill-down: search → artist → release group → release → tracks. + +**Caching strategy:** Cache API responses in SQLite (`mb_cache` table: url, response_json, fetched_at). TTL of 24 hours for search results, 7 days for entity lookups (MusicBrainz data changes infrequently). Cache reduces API calls and improves responsiveness. + +**This is YellowJacket's first network feature** — the app is currently fully offline. Need to handle: network errors gracefully, timeout configuration, offline mode (show cached data), user notification of network status. + +--- + +## 7. Layout Customization System + +### Table Stakes + +| Feature | Why Expected | Complexity | Dependencies | +|---------|--------------|------------|--------------| +| Resizable panels (sidebar, queue, main) | Basic expectation in any multi-panel desktop app. Users want wider sidebar or hidden queue. | MEDIUM | CSS grid/flexbox with drag handles | +| Show/hide queue panel | Already partially implemented (queue toggle button exists) | LOW | Existing queue panel toggle | +| Show/hide sidebar sections | Collapse navigation sections user doesn't need | LOW | Sidebar configuration | +| Persist layout across restarts | Layout changes must survive app restart | LOW | TOML config section | + +### Differentiators + +| Feature | Value Proposition | Complexity | Dependencies | +|---------|-------------------|------------|--------------| +| Section-based component placement (MusicBee-style) | Users choose what goes where — put album art in sidebar, now-playing at top, etc. This is MusicBee's signature feature. | HIGH | Component registry, layout engine, size constraints | +| Component size constraints | Components declare min/max sizes; layout engine respects constraints | MEDIUM | Component metadata system | +| Layout presets | "Compact", "Full", "Mini player" — quick switch between configurations | MEDIUM | Preset definitions + switch mechanism | +| Detachable panels | Pop out queue or now-playing to separate window | HIGH | Wails multi-window support (limited in v2) | + +### Anti-Features + +| Anti-Feature | Why Avoid | What to Do Instead | +|--------------|-----------|-------------------| +| Free-form drag-and-drop layout | Overwhelming complexity, hard to make look good | Section-based: defined slots with selectable components | +| CSS theme editor | Users don't want to write CSS | Extend existing theme system (accent color, background shade) | +| Mobile-responsive layout | This is a desktop app with fixed minimum size | Optimize for 1024x768 minimum | + +### Implementation Notes + +**MusicBee-style layout means section-based composition:** + +The UI is divided into named sections (slots): +- `header` (top bar) +- `sidebar` (left panel) +- `main` (center content area) +- `footer` (bottom bar — now playing + player controls) +- `right-panel` (queue panel or other content) + +Each section has a list of components it can host. Components declare their size constraints (min width/height). Users configure which component goes in which section via a settings UI. + +**Implementation approach:** + +1. **Component registry:** Each component registers itself with metadata (name, description, supported sections, min/max size). This is a TypeScript Map, not a plugin system yet. +2. **Layout configuration:** Stored in TOML config under `[Layout]` section. Maps section names to component names. +3. **Layout renderer:** A root `` component reads config and instantiates the right components in the right sections using dynamic imports. +4. **Resize handles:** CSS resize or custom drag handles on section boundaries. Store widths/heights as percentages in config. + +**Start simple for v1.1:** +- Phase 1: Resizable panels (sidebar width, queue width) with drag handles + persistence +- Phase 2: Show/hide sections + layout presets +- Phase 3: Component-in-section customization (the full MusicBee-style system) + +The full section-based system is the v1.1 "foundation" — functional but not complete. + +**Depends on:** Config system (TOML), existing component architecture, CSS grid layout. + +--- + +## 8. Plugin System + +### Table Stakes + +| Feature | Why Expected | Complexity | Dependencies | +|---------|--------------|------------|--------------| +| Defined plugin API (what plugins can do) | Without clear API boundaries, plugins break on every update | HIGH | API design + stability commitment | +| Plugin loading/unloading | Install/remove plugins without rebuilding the app | HIGH | Dynamic loading mechanism | +| Plugin configuration | Plugins need their own settings that persist | MEDIUM | Extend config system | +| Plugin isolation (one plugin crash doesn't kill app) | Critical for stability | HIGH | Error boundaries, sandboxing | + +### Differentiators + +| Feature | Value Proposition | Complexity | Dependencies | +|---------|-------------------|------------|--------------| +| UI component plugins (custom panels, visualizations) | Plugins can add new views to the layout system | HIGH | Layout customization system + component registry | +| Backend hook plugins (custom metadata sources, scrobblers) | Plugins can intercept/extend backend operations | HIGH | Hook system in Go backend | +| Plugin marketplace/registry | Discover and install plugins | HIGH | External infrastructure | +| TypeScript/JavaScript plugin runtime | Lowest barrier to entry for plugin authors | MEDIUM | Webview already runs JS | + +### Anti-Features + +| Anti-Feature | Why Avoid | What to Do Instead | +|--------------|-----------|-------------------| +| Go plugin system (`plugin` package) | Linux-only, version-fragile, build-tag sensitive, widely considered broken | Use process-based or embedded scripting approach | +| Full filesystem access for plugins | Security nightmare | Sandboxed API with explicit permissions | +| Plugin binary distribution | Build reproducibility, platform issues | Source-based distribution (TypeScript/JS bundles) | +| Network access for plugins without user consent | Privacy concern | Require explicit network permission declaration | + +### Implementation Notes + +**Plugin systems in Go desktop apps are notoriously difficult.** The `plugin` package is Linux-only and requires exact build-tag matching. Wails v2 doesn't have a plugin framework. + +**Recommended approach for v1.1 "foundation":** + +1. **Frontend-first plugins (TypeScript):** + - Plugins are JS/TS bundles loaded dynamically into the webview + - They register with the component registry (layout system) to add UI + - They access backend data through the existing Wails binding layer + - Isolation via Shadow DOM for UI, try/catch for errors + +2. **Backend hooks (Go):** + - Define hook points as interfaces: `OnTrackPlay`, `OnLibraryScan`, `OnMetadataChange`, etc. + - Internal Go "plugins" implement these interfaces + - For v1.1, hooks are compile-time (not dynamic) — the plugin system defines the API, but plugins are compiled in + - Dynamic loading deferred to future (hashicorp/go-plugin RPC, or WASM) + +3. **Plugin manifest:** + ```json + { + "name": "my-plugin", + "version": "1.0.0", + "description": "Does a thing", + "entry": "index.js", + "hooks": ["onTrackPlay", "onLibraryScan"], + "ui": [{"component": "my-panel", "sections": ["sidebar", "right-panel"]}], + "permissions": ["network"] + } + ``` + +4. **Plugin directory:** `~/.config/yellowjacket/plugins//` containing manifest + JS bundle + +**v1.1 scope should be the API definition and loading mechanism** — not a full marketplace. "Working foundation" means: plugins can be loaded, they can register UI components, they can subscribe to backend events. The API surface is deliberately small and stable. + +**Depends on:** Layout customization system (for UI plugins), event system (for hook subscriptions), config system (for plugin settings). --- ## Feature Dependencies ``` -[Fix SetContext races] - └── (no deps — standalone fix) - -[Fix error handling gaps (MPRIS, artist credit, config perms)] - └── (no deps — standalone fixes) - -[Separate scan warnings from fatal errors] - └── (no deps — changes Library.Scan return signature) - -[Add in-memory SQLite test infrastructure] - └──requires──> [database.NewTestDB() helper] - └──enables──> [Queue unit tests] - └──enables──> [Library unit tests] - └──enables──> [Database layer tests] - └──enables──> [Config tests] - -[Extract testable player logic] - └── (no deps — pure function extraction) - └──enables──> [Player pure logic tests] - -[FTS5 query consolidation (SQL view)] - └──should-precede──> [Database layer tests] - (test the consolidated queries, not the duplicated ones) - -[Queue persistence optimization (incremental updates)] - └──should-precede──> [Queue unit tests] - (test the optimized persistence, not the DELETE-ALL pattern) - -[Library store lazy loading] - └── (no deps — remove eagerFetch() call) - -[SetQueue Phase 2 optimization] - └──requires──> [Queue unit tests] - (need tests to verify the optimization doesn't break resolution) - -[Event name parity validation] - └── (no deps — standalone build-time check) - -[UI polish / transitions] - └── (no deps — CSS-only or Lit reactive changes) - -[Frontend rendering optimization] - └──benefits-from──> [Library store lazy loading] - (less data in memory = faster re-renders) +Scan Cancellation ──── (standalone, no dependencies) + │ +Tag Editing ────────── (standalone, needs new libraries) + │ +Smart Playlists ────── depends on: existing DB/track_metadata VIEW + │ +Keyboard Shortcuts ─── (standalone, frontend-primary) + │ +Gapless + Crossfade ── depends on: audio pipeline refactor + │ +MusicBrainz Browser ── depends on: HTTP client (new), network handling (new) + │ +Layout Customization ── depends on: component registry (new) + │ +Plugin System ──────── depends on: Layout Customization, Event system, Config system ``` -### Dependency Notes - -- **Test infrastructure is the critical enabler:** Almost all other improvements benefit from having tests first (to verify refactoring safety) or should happen before tests (to test the right code). The ordering matters: fix persistence patterns *before* writing persistence tests, consolidate SQL *before* writing SQL tests. -- **Concurrency fixes are independent:** They're small, self-contained, and should be done first — they represent known correctness issues. -- **Performance optimizations benefit from tests:** The queue persistence optimization and SetQueue Phase 2 fix both modify core queue logic. Having queue tests first provides a safety net. -- **Frontend work is independent of backend work:** Library store lazy loading, UI polish, and rendering optimization don't depend on backend changes. +**Dependency ordering (what blocks what):** +1. **Nothing blocks:** Scan cancellation, tag editing, keyboard shortcuts, smart playlists, MusicBrainz browser +2. **Layout blocks plugins:** Plugin UI registration needs the layout component registry +3. **Gapless is self-contained** but is the highest-risk change (audio pipeline) --- -## Prioritization +## MVP Recommendation -### Phase 1: Correctness & Test Foundation (Do First) +### Build First (low risk, high value, unblocked) +1. **Scan cancellation** — lowest complexity, immediate UX win, architecture already supports it +2. **Keyboard shortcuts** — low complexity, massive usability improvement, no backend changes +3. **Smart playlists** — medium complexity, high value, builds on existing DB infrastructure -Fixes known bugs and establishes the test infrastructure that makes everything else safe. +### Build Second (medium risk, foundational) +4. **Tag editing** — medium complexity, requires new dependencies, needed before MusicBrainz becomes useful +5. **MusicBrainz browser** — medium complexity, first network feature, independent of others +6. **Layout customization** — medium-high complexity, needed before plugins -- [ ] Fix SetContext data races (Queue, Library, Playlist) — LOW effort, HIGH value -- [ ] Fix package-level `startupErr` → struct field — LOW effort -- [ ] Fix config file permissions — LOW effort -- [ ] Fix swallowed errors (MPRIS, artist credit) — LOW effort -- [ ] Separate scan warnings from fatal errors — MEDIUM effort -- [ ] Create in-memory SQLite test helper (`database.NewTestDB()`) — MEDIUM effort -- [ ] Extract testable player pure logic (volume, state) — LOW effort +### Build Last (high risk, high complexity) +7. **Gapless playback + crossfade** — highest complexity, fundamental audio pipeline change, can ship independently +8. **Plugin system** — highest complexity, depends on layout system, explicitly a "foundation" for v1.1 -### Phase 2: SQL & Performance Foundations (Do Second) +### Defer (explicitly) +- Tag-to-filename rename +- Undo/redo for tag edits +- Play count tracking (needed for some smart playlist rules) +- Rating system +- Plugin marketplace +- Dynamic Go plugin loading +- Detachable panels (Wails v2 limitation) -Improves the code that tests will be written against. - -- [ ] Consolidate FTS5 JOIN pattern (SQL view or constant) — MEDIUM effort -- [ ] Migrate queue lookups to `sqlc.slice()` — MEDIUM effort -- [ ] Optimize queue persistence (incremental updates) — MEDIUM effort -- [ ] Fix SetQueue Phase 2 redundant lookups — LOW effort -- [ ] Remove `eagerFetch()` from library store constructor — LOW effort - -### Phase 3: Comprehensive Tests (Do Third) - -Tests verify the improved code from Phases 1-2. - -- [ ] Queue unit tests (SetQueue, navigation, shuffle, repeat, persistence) — HIGH effort -- [ ] Library scan unit tests (metadata, entity cache, orphan cleanup) — HIGH effort -- [ ] Database layer tests (FTS5 queries, migrations) — MEDIUM effort -- [ ] Config tests (load/save roundtrip, validation, defaults) — MEDIUM effort -- [ ] Player pure logic tests (volume math, state serialization) — LOW effort -- [ ] Event name parity test — LOW effort - -### Phase 4: Polish & Frontend (Do Last) - -Visual and frontend improvements that don't affect backend correctness. - -- [ ] UI transitions and responsive feedback — MEDIUM effort -- [ ] Frontend rendering optimization (repeat directive, debounced notifications) — MEDIUM effort -- [ ] Document intentional exceptions (hand-crafted SQL, singleton store lifecycle) — LOW effort - -## Feature Prioritization Matrix - -| Improvement | Reliability Value | Implementation Cost | Priority | -|-------------|-------------------|---------------------|----------| -| Fix SetContext data races | HIGH | LOW | **P1** | -| Fix startupErr, config perms | HIGH | LOW | **P1** | -| Fix swallowed errors | HIGH | LOW | **P1** | -| Separate scan warnings/errors | HIGH | MEDIUM | **P1** | -| In-memory SQLite test helper | HIGH | MEDIUM | **P1** | -| Extract testable player logic | MEDIUM | LOW | **P1** | -| FTS5 query consolidation | MEDIUM | MEDIUM | **P2** | -| Queue persistence optimization | MEDIUM | MEDIUM | **P2** | -| SetQueue Phase 2 fix | MEDIUM | LOW | **P2** | -| Library store lazy loading | MEDIUM | LOW | **P2** | -| Queue unit tests | HIGH | HIGH | **P2** | -| Library unit tests | HIGH | HIGH | **P2** | -| Database tests | MEDIUM | MEDIUM | **P2** | -| Config tests | MEDIUM | MEDIUM | **P2** | -| Event name parity validation | MEDIUM | LOW | **P2** | -| Player pure logic tests | MEDIUM | LOW | **P2** | -| UI transitions / polish | LOW | MEDIUM | **P3** | -| Frontend rendering optimization | LOW | MEDIUM | **P3** | -| Migrate queue SQL to sqlc | LOW | MEDIUM | **P3** | - -**Priority key:** -- P1: Must do — correctness issues or critical enablers -- P2: Should do — significant quality improvement -- P3: Nice to have — polish, can defer if time-constrained +--- ## Sources -- Go race detector: https://go.dev/doc/articles/race_detector — HIGH confidence (official Go docs) -- sqlc `sqlc.slice()` for SQLite: https://docs.sqlc.dev/en/stable/reference/macros.html — HIGH confidence (official sqlc docs, verified via WebFetch) -- sqlc batch operations: https://docs.sqlc.dev/en/stable/howto/select.html#mysql-and-sqlite — HIGH confidence (official docs) -- Lit `repeat` directive: https://lit.dev/docs/templates/lists/#the-repeat-directive — HIGH confidence (official Lit docs, verified via WebFetch) -- Lit rendering model: https://lit.dev/docs/components/rendering/ — HIGH confidence (official docs) -- `@lit-labs/virtualizer` — already in use in codebase (track-list, cover-grid) -- `modernc.org/sqlite` in-memory DB — HIGH confidence (`:memory:` is standard SQLite, driver already in deps) -- Go `errors.Join()` — HIGH confidence (standard library since Go 1.20, already used in codebase) -- Go mutex patterns — HIGH confidence (standard library, matches existing codebase conventions) - ---- -*Feature research for: YellowJacket consolidation milestone* -*Researched: 2026-02-27* +- MusicBrainz API documentation: https://musicbrainz.org/doc/MusicBrainz_API (HIGH confidence — official docs, verified 2026-03-06) +- MusicBrainz rate limiting: https://musicbrainz.org/doc/MusicBrainz_API/Rate_Limiting (HIGH confidence — official docs) +- `github.com/bogem/id3v2/v2` v2.1.4: https://pkg.go.dev/github.com/bogem/id3v2/v2 (HIGH confidence — official pkg.go.dev) +- `github.com/go-flac/go-flac` v1.0.0: https://pkg.go.dev/github.com/go-flac/go-flac (HIGH confidence — official pkg.go.dev) +- `github.com/go-flac/flacvorbis` v0.2.0: https://pkg.go.dev/github.com/go-flac/flacvorbis (HIGH confidence — official pkg.go.dev) +- `github.com/gopxl/beep/v2` v2.1.1: https://pkg.go.dev/github.com/gopxl/beep/v2 (HIGH confidence — official pkg.go.dev, confirms Mixer, Seq, Loop2, effects) +- YellowJacket codebase analysis: `.planning/codebase/` (HIGH confidence — direct code inspection) +- Desktop music player patterns: foobar2000, MusicBee, Strawberry, Deadbeef, Audacious (MEDIUM confidence — training data knowledge of established players) diff --git a/.planning/research/PITFALLS.md b/.planning/research/PITFALLS.md index 8e81fe5..202b92a 100644 --- a/.planning/research/PITFALLS.md +++ b/.planning/research/PITFALLS.md @@ -1,288 +1,414 @@ -# Pitfalls Research +# Domain Pitfalls -**Domain:** Go/Wails/SQLite Desktop Music Player — Consolidation & Refactoring -**Researched:** 2026-02-27 -**Confidence:** HIGH (based on codebase analysis + established Go/SQLite patterns) +**Domain:** Adding tag editing, scan cancellation, smart playlists, customizable keyboard shortcuts, gapless playback + crossfade, MusicBrainz browser, layout customization, and plugin system to an existing Go/Wails/Lit/SQLite desktop music player +**Researched:** 2026-03-06 +**Confidence:** HIGH (based on deep codebase analysis, official MusicBrainz API docs, beep library docs, and established Go/Wails/SQLite patterns) + +--- ## Critical Pitfalls -### Pitfall 1: Refactoring Concurrency Without Tests Creates Invisible Regressions - -**What goes wrong:** -You fix a data race (e.g., adding `q.mu.Lock()` to `Queue.SetContext()`) and the fix itself introduces a deadlock because you didn't understand the full call graph. Alternatively, the race fix changes timing semantics that other code implicitly depended on (e.g., Phase 2 of `SetQueue` now acquires the lock at a different time relative to `playCurrentTrack()`). Because there are no tests, the regression only manifests during specific usage patterns — like quickly switching playlists while a background resolve is running. - -**Why it happens:** -The instinct is "add mutex → race fixed." But mutexes change scheduling behavior. In YellowJacket, the Player has a documented lock ordering (`p.mu` before `speaker.Lock()`), the Queue has a generation counter pattern with `setQueueGen`, and the beep callback dispatches to a goroutine. These are three interacting concurrency mechanisms. Adding a lock to one path changes how the other two paths interleave. - -**How to avoid:** -1. **Write characterization tests first for the non-racy behavior.** Before fixing the race in `Queue.SetContext()`, write tests that verify `SetQueue` → `resolveRemainingTracks` → `emitQueueChanged` produces correct results. These tests won't catch the race (they're single-goroutine), but they'll catch if your mutex addition breaks the non-concurrent path. -2. **Fix races in a specific order:** First fix `SetContext()` patterns (they're called once during startup, lowest risk). Then fix the Queue mutation paths. Leave the Player's dual-lock pattern for last — it's the most complex and already works correctly. -3. **Use `go test -race` on every change.** Build a test binary with `-tags webkit2_41 -race` and run it. The race detector will confirm fixes and catch new races. -4. **Map the lock acquisition graph before adding any mutex.** For each public method, trace which locks it acquires and which callbacks it invokes. The `onPlaybackFinished()` goroutine dispatch (player.go line 350) is the critical pattern — it exists specifically to break a lock cycle. - -**Warning signs:** -- App hangs/freezes after a refactoring change (deadlock) -- "Previous" or "Next" track skips incorrectly after rapid clicks -- Queue panel briefly shows wrong tracks then corrects itself -- `-race` flag reports on code paths you didn't change - -**Phase to address:** -Testing phase should come first — write tests for queue operations, then fix concurrency. Specifically: (1) characterization tests for queue, (2) fix SetContext races, (3) fix mutation races, (4) fix player double-lock. +These mistakes cause rewrites, data loss, or architectural dead ends. --- -### Pitfall 2: SQLite In-Memory Tests Behave Differently From File-Based Production DB +### Pitfall 1: Tag Writing Corrupts Audio Files or Loses Data -**What goes wrong:** -You write tests using `:memory:` SQLite and they pass. In production with a file-based WAL-mode database and `SetMaxOpenConns(1)`, the behavior differs. Common divergences: -- `:memory:` doesn't persist `PRAGMA foreign_keys = ON` across connections (each new connection starts with FK enforcement off) -- `:memory:` with `SetMaxOpenConns(1)` doesn't surface contention the way file-based does (because there's only one connection, it never blocks — same as production, but WAL checkpoint behavior differs) -- FTS5 `search_index` tokenization may behave differently if the test doesn't apply the same schema setup sequence as `NewDB()` -- `PRAGMA user_version` is per-connection for `:memory:`, so migration tests that open a second connection see version 0 +**Feature area:** Tag editing +**What goes wrong:** Writing ID3/Vorbis tags corrupts the audio file — partial writes leave the file unplayable, or the tag library strips existing frames (cover art, replay gain, MusicBrainz IDs) that it doesn't understand. The user edits "Artist" and loses their embedded lyrics, custom TXXX frames, and cover art. Worse: if the file is currently being played by beep, simultaneous reads and writes corrupt both the playback stream and the tag data. **Why it happens:** -`:memory:` is faster and doesn't leave test artifacts, so it's the default choice. But SQLite's `:memory:` is a distinct database per connection, not per DSN. The production code opens a file with specific pragmas (`_busy_timeout=5000&_journal_mode=WAL`), `PRAGMA foreign_keys = ON`, and runs schema files in alphabetical order. Any test that doesn't replicate this sequence is testing a different database. +- `github.com/dhowden/tag` (already in deps) is **read-only** — it does not support tag writing. The CONCERNS.md notes tag writing as a known gap (line 22). +- ID3v2 tag writing requires rewriting the file header. If the new tag is larger than the existing padding, the **entire file must be rewritten** — the audio data shifts. A crash or power loss during rewrite produces a corrupted file. +- FLAC uses Vorbis Comments in a METADATA_BLOCK. Rewriting this block similarly requires shifting the audio frame data if the block grows. +- The beep decoder holds an `*os.File` handle for the currently playing track. Writing to that same file while beep's read-ahead goroutine (`BufferedStreamer.readAhead`) is actively streaming from it will cause data corruption — the file offsets shift but the decoder's internal state doesn't update. -**How to avoid:** -1. **Create a test helper that mirrors `NewDB()` exactly:** Open a temp file (`t.TempDir() + "/test.db"`), apply the same pragmas, run the same embedded schemas, run `runMigrations()`. Export a `NewTestDB(t *testing.T) *DB` helper. -2. **Use `t.TempDir()`** — Go cleans it up automatically. This is enforced by the `usetesting` linter already configured. -3. **Always set `PRAGMA foreign_keys = ON`** in the test helper — the production code does this, and cascade deletes (like `queue_tracks` → `audio_files`) depend on it. -4. **If you do use `:memory:` for pure unit tests** (testing a single query), use the DSN `file::memory:?cache=shared` and document that it won't test WAL behavior. +**Prevention:** +1. **Use `github.com/bogem/id3v2/v2` (n10v/id3v2)** for MP3 tag writing — it supports ID3v2.3 and v2.4 read/write with 359 stars and active maintenance. For FLAC, use `github.com/go-flac/flacvorbis` or a similar FLAC-specific writer. Keep `dhowden/tag` for read operations. +2. **Write-to-temp-then-rename pattern:** Write the modified file to a temp file in the same directory, then `os.Rename()` atomically. This ensures the original file is never partially written. On failure, the temp file is deleted and the original is untouched. +3. **Block tag writes on the currently playing file.** Before writing, check if `player.currentFile` points to the same path. If so, either: (a) stop playback, close the file, write, then reload; or (b) queue the write to execute after the track changes. Option (a) is simpler and more predictable. +4. **After writing tags, update the database.** The tag write changes the file on disk but the SQLite database still has the old metadata. You must: update the `recordings` table, update `artist_credit`/`release_groups` if changed, rebuild the FTS5 `search_index` entry for that track, and invalidate any entity cache. +5. **Preserve frames you don't edit.** When using id3v2, open with `Parse: true` to load all existing frames, modify only the ones the user changed, then save. Don't create a new tag from scratch. -**Warning signs:** -- Tests pass but `ON DELETE CASCADE` doesn't fire in production -- FTS5 queries return different results in tests vs. app -- Migration tests pass but real migrations fail on existing databases -- Queue persistence tests pass but tracks are lost on restart +**Detection:** +- Audio file won't play after tag edit +- Cover art disappears after editing title/artist +- Playback glitches or crashes during a tag write on the currently playing file +- FTS5 search returns stale metadata after edits -**Phase to address:** -First phase — the test infrastructure setup. `NewTestDB()` must be correct before any database tests are written. +**Confidence:** HIGH — `dhowden/tag` being read-only is confirmed by its API (no `Save()` or `Write()` methods). File corruption from concurrent read/write is a fundamental OS-level concern. --- -### Pitfall 3: Deadlock From Player mutex + speaker.Lock() Ordering Violation +### Pitfall 2: Gapless Playback Breaks the Existing Lock Ordering and Callback Contract -**What goes wrong:** -The Player has a critical invariant: always acquire `p.mu` before `speaker.Lock()`. The beep library's playback callback runs with the speaker lock held. If you refactor a method to call `speaker.Lock()` while holding `p.mu` in a way that blocks, and the callback tries to acquire `p.mu`, you get a classic ABBA deadlock: -- Goroutine 1: holds `p.mu`, waiting for `speaker.Lock()` -- Goroutine 2 (beep callback): holds speaker lock, goroutine dispatch calls `onPlaybackFinished()` which waits for `p.mu` - -Currently this is avoided by the `go p.onPlaybackFinished()` dispatch pattern (player.go line 350), which means the callback itself doesn't hold `p.mu` — it just launches a goroutine. But the `startPaused()` method (line 340-354) acquires `speaker.Lock()` while `p.mu` is held by the caller. This works because it's a non-blocking lock/unlock sequence — but if you move speaker operations into a new method without understanding the lock context, deadlock follows. +**Feature area:** Gapless playback + crossfade +**What goes wrong:** The current playback flow uses `beep.Seq(streamer, beep.Callback(func() { go p.onPlaybackFinished() }))` — when the stream ends, the callback fires (with speaker lock held), dispatches to a goroutine, which then tells the queue to advance, which calls `player.LoadFile()`. This produces an audible gap of 100-500ms (file open + decode + resample + buffer fill). Attempting to eliminate this gap by pre-decoding the next track while the current one plays introduces a new concurrent resource: two open decoders, two BufferedStreamers, two file handles, and a crossfade mixer that must be swapped into the speaker chain atomically. **Why it happens:** -Refactoring moves code between methods. If you extract `startPaused()` into a helper or inline it into another method, you might accidentally change the lock nesting. The `speaker.Lock()/Unlock()` inside `startPaused()` is safe because it's called with `p.mu` held (correct ordering), but `speaker.Play()` on line 347 is called with `p.mu` held too — and that's where the callback is registered. If the callback fires immediately (e.g., for a zero-length stream), the goroutine dispatch is the only thing preventing deadlock. +- **beep's `speaker.Play()` adds streamers to a global mix.** You can call it multiple times — new streamers are mixed with existing ones. But the Player struct assumes a single active streamer chain (`p.speakerStreamer`). Pre-loading a second track means two streamer chains are live simultaneously. +- **The lock ordering `p.mu → speaker.Lock()` assumes one-at-a-time.** With crossfade, you need to: (a) decode the next track under `p.mu`, (b) build its streamer chain, (c) under `speaker.Lock()`, splice the crossfade mixer into the active chain. If the existing track's `onPlaybackFinished` fires during this splice, you have a race between the callback goroutine (acquiring `p.mu`) and the pre-load logic (holding `p.mu` and needing `speaker.Lock()`). +- **The `BufferedStreamer` has its own goroutine.** With two tracks buffering simultaneously, you have two `readAhead()` goroutines competing for disk I/O. The `Close()` method must be called on the old BufferedStreamer at the right time — too early truncates audio, too late leaks goroutines. -**How to avoid:** -1. **Never refactor player lock code without drawing the lock acquisition graph first.** Document which methods hold which locks at each point. -2. **Keep the `go p.onPlaybackFinished()` dispatch pattern.** Never change this to a direct call. Add a comment explaining why. -3. **Extract pure logic (volume math, state serialization) into lock-free functions** that can be tested independently. Don't extract methods that need to hold locks. -4. **Add a regression test** that rapidly calls `LoadFile` → `Play` → `LoadFile` → `Play` to exercise the callback timing. Even without hardware, this can be tested with a mock streamer. +**Prevention:** +1. **Don't try to pre-decode inside the existing `LoadFile` flow.** Instead, build a separate pre-loading mechanism: when the current track reaches N seconds from the end (detectable by comparing `seeker.Position()` to `seeker.Len()`), start decoding the next track in a background goroutine. Store the pre-decoded streamer and format in a `nextTrack` field on the Player struct, protected by `p.mu`. +2. **For gapless (no crossfade): use `beep.Seq` with both streamers.** When the pre-decoded next track is ready, replace the current speaker chain with `beep.Seq(remainingCurrentTrack, nextTrackStreamer, beep.Callback(...))`. This lets beep handle the seamless transition without a gap. The key insight: you must resample both tracks to the same sample rate (the speaker rate, 44100) before sequencing them. +3. **For crossfade: build a custom `CrossfadeStreamer`.** This streamer reads from both the ending track and the starting track simultaneously, mixing their samples with a volume ramp. Register this single crossfade streamer with the speaker. It internally manages the two underlying streamers and their lifecycle. +4. **Never close the outgoing `BufferedStreamer` until the crossfade is complete.** The crossfade streamer should call `Close()` on the old track's BufferedStreamer only after it has drained all needed samples from it. +5. **The `onPlaybackFinished` callback must be suppressed during gapless/crossfade transitions.** If beep's `Seq` fires the callback for track A while you've already started track B, the queue will try to advance again. Use a "gapless transition in progress" flag, or change the callback to a no-op during transitions and notify the queue directly from the pre-load logic. -**Warning signs:** -- App freezes when track finishes naturally (not when user clicks Next) -- App freezes specifically when rapidly changing tracks -- `SIGQUIT` goroutine dump shows both `p.mu.Lock()` and `speaker.Lock()` in different goroutines' stacks +**Detection:** +- App deadlocks when tracks transition (lock ordering violation) +- Two tracks play simultaneously (both speaker.Play'd without removing the old one) +- Goroutine leak (BufferedStreamer.readAhead never returns) +- Audio cuts out briefly then resumes (old track closed before crossfade samples drained) +- Queue advances twice (callback fires AND pre-load logic notifies queue) -**Phase to address:** -Player refactoring phase. Extract testable pure logic first, leave lock-sensitive code paths for last. Document the lock ordering invariant with a test that validates the goroutine dispatch pattern. +**Confidence:** HIGH — lock ordering and callback contract are documented in player.go. The `go p.onPlaybackFinished()` goroutine dispatch pattern is explicitly commented as avoiding deadlock (lines 355-361). --- -### Pitfall 4: FTS5 Query Consolidation Breaks Search Ranking or Returns +### Pitfall 3: Scan Cancellation Leaves Database in Inconsistent State -**What goes wrong:** -You consolidate the 5+ copies of the FTS5 JOIN pattern into a shared constant or query builder. The consolidated query subtly differs from one of the originals — maybe a `LEFT JOIN` becomes an `INNER JOIN`, or the `COALESCE` default changes from `''` to `NULL`, or the subquery for `release_group_recordings` uses `MAX` instead of `MIN`. Search results change: tracks without albums stop appearing, or ranking changes because FTS5's `rank` function scores differently when join columns are NULL vs empty string. +**Feature area:** Scan cancellation +**What goes wrong:** User cancels a scan mid-way through Phase 4 (DB writer batching results). The current batch may be partially committed — 30 of 50 files written in a transaction that got rolled back, but the `added` counter was already incremented. Or worse: the orphan cleanup (Phase 5) runs on a partial scan, deleting files from the database that weren't visited because the walk was cancelled early, not because they were actually deleted from disk. **Why it happens:** -The 5 copies look identical but have small contextual differences. `SearchFTS` uses `ORDER BY rank`, `SearchFTSTracks` might have a different LIMIT, `RebuildSearchIndex` doesn't need the rank column at all. When consolidating, you pick one version as the "canonical" form and the others silently regress. Additionally, FTS5's ranking is sensitive to which columns contain data — a `COALESCE` that returns `''` instead of the actual NULL affects the `bm25()` algorithm differently. +- The scan pipeline has 6 phases running as communicating goroutines (walk → worker pool → DB writer → orphan cleanup → thumbnail generation). Cancellation must propagate cleanly through all of them. +- The `l.ctx.Done()` checks in the walk phase (lines 297, 324) use the Wails app context, which is only cancelled on shutdown. A user-triggered cancellation needs a separate `context.WithCancel()`. +- The `existingPaths` sync.Map is loaded in Phase 1 and entries are removed as files are found during the walk (Phase 2). Orphan cleanup (Phase 5) iterates remaining entries and deletes them. If the walk was cancelled early, many valid files remain in `existingPaths` and get incorrectly deleted as orphans. +- The DB writer's `flushBatch()` runs inside a transaction. If the context is cancelled between `BEGIN` and `COMMIT`, the transaction rolls back, but the import results have already been dequeued from `resultChan` — they're lost. -**How to avoid:** -1. **Write search tests BEFORE consolidating.** Test each current function with known data: a track with full metadata, a track with no artist, a track with no album, a track matched only by file path. Capture the exact result set and ranking order. -2. **Consolidate the JOIN clause only, not the full query.** Extract the `FROM ... JOIN` chain as a SQL fragment constant. Let each function keep its own SELECT, WHERE, and ORDER BY clauses. -3. **Verify FTS5 `INSERT INTO search_index` uses the same column values as the search queries.** If the index stores `COALESCE(r.name, '')` but the search query expects `r.name`, the match behavior differs. -4. **Run the consolidation as a pure refactor with zero-diff tests** — if any test changes results, the consolidation introduced a bug. +**Prevention:** +1. **Create a scan-specific context:** `scanCtx, scanCancel := context.WithCancel(l.ctx)`. Store `scanCancel` on the Library struct so the frontend can call a `CancelScan()` method. +2. **Skip orphan cleanup on cancelled scans.** Add a `cancelled bool` check before Phase 5. If the scan was cancelled, the `existingPaths` map is incomplete — orphan cleanup would delete valid files. Emit a `LibraryScanCancelled` event instead of `LibraryScanComplete`. +3. **Make the DB writer respect cancellation between batches, not mid-batch.** Check `scanCtx.Done()` in the `for result := range resultChan` loop, but let the current `flushBatch()` complete before stopping. This ensures each committed batch is complete. +4. **Drain channels on cancellation.** When the walk is cancelled, it closes `workChan`. Workers drain and close `resultChan`. The DB writer drains `resultChan` normally. But if workers are blocked sending to `resultChan` (buffer full), they need to select on `scanCtx.Done()` too. Ensure all goroutines can unblock. +5. **Report partial results.** The `ScanMetrics` should include a `Cancelled: true` flag. The frontend should show "Scan cancelled — X files processed" rather than treating it as a failure. -**Warning signs:** -- Search returns fewer results than before -- Search ranking changes (previously top result now buried) -- Tracks with missing metadata (no artist, no album) disappear from search -- `RebuildSearchIndex` produces different results than incremental inserts +**Detection:** +- Files disappear from library after cancelling a scan (orphan cleanup ran on partial data) +- `ScanMetrics.Added` doesn't match actual DB row count (counter incremented but batch rolled back) +- App hangs on cancel (goroutines blocked on channel sends/receives) +- Subsequent scan adds files that were already in the library (previous scan's partial results lost) -**Phase to address:** -Database/code quality phase. Write FTS5 search tests first, then consolidate. +**Confidence:** HIGH — confirmed by reading the scan pipeline code (library.go lines 175-540). The orphan cleanup problem is the most dangerous because it's a silent data loss. --- -### Pitfall 5: Eager-to-Lazy Library Loading Creates Visible UX Regression +### Pitfall 4: Plugin System Without Isolation Crashes the Host App -**What goes wrong:** -You change `libraryStore` from eager-fetching all data on construction to lazy-loading per view. The first time the user navigates to the tracks view, there's a loading delay that didn't exist before. The cover grid flickers as albums load in chunks. Worse: components that used synchronous `getCachedTracks()` (which previously always returned data because of eager fetch) now return `null` and render empty states. The user, who has been using this app daily with instant library display, perceives this as a regression. +**Feature area:** Plugin system +**What goes wrong:** A plugin panics in a goroutine, and since Go panics are per-goroutine, the entire application crashes. Or a plugin holds the speaker lock for too long and audio glitches. Or a plugin writes to the SQLite database concurrently and hits `SQLITE_BUSY`. Or a plugin registers a Wails event handler that conflicts with core event names. The "full-access API" promised in the project requirements makes every component a potential victim of plugin misbehavior. **Why it happens:** -The current `eagerFetch()` fires all four fetches (`getTracks`, `getAlbums`, `getArtists`, `getGenres`) in the constructor. By the time the user interacts, data is already cached. Switching to lazy loading means the first interaction hits an async boundary. Every component that calls `getCachedTracks()` synchronously (used by at least `track-list`, `cover-grid`, `playlist-view`) will get `null` on first render and must handle a loading state that was previously invisible. +- Go has no built-in process isolation for plugins. `plugin.Open()` loads shared objects into the same address space. Panics, goroutine leaks, and memory corruption in plugins affect the host. +- The SQLite single-writer constraint (`SetMaxOpenConns(1)`) means any plugin database access serializes with all core operations. A slow plugin query blocks library scans, queue persistence, and player state saves. +- The Wails event system is a global namespace. If a plugin emits `TrackChanged`, it could confuse the frontend. If it subscribes to `PlaybackFinished`, it runs in the same goroutine context as core handlers. -**How to avoid:** -1. **Keep eager fetch for the initial view.** If the user's default view is "tracks," fetch tracks eagerly and lazy-load the rest. The library store already has the lazy `getTracks()` / `getAlbums()` pattern with `tracksLoading` / `albumsLoading` flags — the issue is that `eagerFetch()` triggers them all. -2. **Audit every `getCachedTracks()` / `getCachedAlbums()` call site.** Each one needs a loading state or skeleton UI. Don't change the store without updating all consumers. -3. **Measure before optimizing.** Profile the actual startup time with a large library. If `GetAllTracks()` takes 200ms for 50k tracks, that's fast enough to keep eager. The bottleneck might be rendering, not fetching. -4. **If lazy loading, implement skeleton/shimmer states** that feel faster than the current blank-then-populate pattern. The perceived performance matters more than actual latency. +**Prevention:** +1. **Don't use Go's `plugin` package.** It requires matching Go versions between host and plugin, doesn't work on all platforms, and provides no isolation. Instead, use one of: + - **Embedded scripting (Lua via `github.com/yuin/gopher-lua` or JavaScript via `github.com/nicholasgasior/goja`):** Run plugin code in an interpreter with controlled API exposure. Panics in the interpreter don't crash the host. + - **Process-based plugins with gRPC/stdin-stdout RPC:** Like HashiCorp's `go-plugin` model. Full isolation but higher complexity and latency. + - **WASM plugins (e.g., `github.com/tetratelabs/wazero`):** Good isolation, cross-platform, but limited Go interop. + For a desktop music player, **embedded Lua or JS is the pragmatic choice** — it's fast enough for UI customization and event hooks, and panics are contained. +2. **Wrap all plugin API calls in recover().** If using native Go plugins or any host-side callback, wrap in `defer func() { if r := recover(); r != nil { log.Error(...) } }()`. +3. **Give plugins a read-only database view.** Open a second read-only SQLite connection (since WAL mode supports concurrent readers) for plugins. This doesn't compete with the single writer. +4. **Namespace plugin events.** All plugin-emitted events must be prefixed: `plugin::`. Core events cannot be emitted by plugins. +5. **Rate-limit plugin API calls.** A plugin calling `Player.Seek()` in a tight loop would create a cascade of mutex acquisitions, speaker locks, event emissions, and frontend updates. Apply a rate limiter (e.g., 10 calls/second per plugin per API surface). -**Warning signs:** -- Empty track list visible for a fraction of a second on app start -- Cover grid shows placeholder then jumps as albums load -- Components flash between empty and populated states -- User says "it feels slower" even if total time is the same +**Detection:** +- App crashes with panic stack trace originating in plugin code +- Audio stutters when a plugin is active (speaker lock contention) +- Library scan takes 10x longer with plugins installed (SQLite writer contention) +- Frontend shows ghost events from plugin event namespace collisions -**Phase to address:** -Performance phase. Profile first, then decide whether lazy loading is actually needed. If yes, update all consumer components in the same change. +**Confidence:** MEDIUM — plugin architecture is a design decision with many valid approaches. The specific pitfalls around Go's `plugin` package and SQLite single-writer are HIGH confidence. The recommendation for embedded scripting is based on the "foundation, not feature-complete" goal stated in PROJECT.md. --- -### Pitfall 6: Queue Persistence Migration Loses Queue State +## Moderate Pitfalls -**What goes wrong:** -You change queue persistence from full-rewrite (`DELETE + INSERT ALL`) to incremental (`INSERT/DELETE individual rows`). The schema or persistence format changes. The user restarts the app and their queue is empty because the new `RestoreState()` can't read the old format, or the migration from full-rewrite to incremental left the `queue_tracks` table in an inconsistent state (e.g., duplicate positions, missing foreign keys). - -**Why it happens:** -The current `persistTracks()` does `DELETE FROM queue_tracks` + batch INSERT inside a transaction. This is a clean slate every time — position values are always sequential and consistent. An incremental approach must maintain position ordering through individual INSERT/DELETE/UPDATE operations. If you change the persistence strategy without migrating existing data, or if the new code assumes positions are always contiguous when the old code may have left gaps, the restore fails. - -**How to avoid:** -1. **The new persistence code must be able to read the old format.** The `queue_tracks` table has `(id, audio_file_id, position)`. As long as you don't change the schema, `RestoreState()` works unchanged. Only change the write path. -2. **Write a test that persists with the old method, then restores with the new method.** This is the backward compatibility test. -3. **Keep the full-rewrite as a fallback** for `SetQueue` (which replaces the entire queue anyway). Only use incremental for `AddTrack`, `RemoveTrack`, and `MoveTrack`. -4. **Validate position ordering after every incremental mutation** in debug builds. Assert that positions are monotonically increasing. - -**Warning signs:** -- Queue is empty after app restart -- Queue tracks are in wrong order after restart -- `RestoreState` logs errors about missing audio files -- Queue tracks have duplicate or negative positions - -**Phase to address:** -Performance phase. Write queue persistence tests first, then change the write strategy. +These mistakes cause significant rework or user-facing bugs but not architectural collapse. --- -### Pitfall 7: Wails Binding Regeneration Silently Breaks Frontend After Go Struct Changes +### Pitfall 5: MusicBrainz Rate Limiting Blocks the User or Gets the App Banned -**What goes wrong:** -You rename a Go struct field (e.g., `queue.Track.Position` → `queue.Track.SortOrder`), change a method signature, or add a new exported method to a bound struct. The Wails binding generator creates new TypeScript files in `frontend/wailsjs/go/`, but the generated types don't match what the frontend code expects. The TypeScript compiler may or may not catch this depending on whether the frontend uses the generated types or inline types. If the frontend uses `any` casts or untyped event payloads, the mismatch is silent. +**Feature area:** MusicBrainz browser +**What goes wrong:** The app fires burst requests to MusicBrainz when the user browses an artist's discography (artist lookup + release groups + releases + recordings = 4+ API calls per click). MusicBrainz enforces a **1 request per second per IP address** rate limit (confirmed from official docs). Exceeding this returns HTTP 503 for ALL subsequent requests until the rate drops. The user sees blank pages and errors. Worse: if the User-Agent string is missing or generic, the app falls into the "anonymous" throttle bucket with a shared 50 req/s global limit. **Why it happens:** -Wails v2 binding generation (`wails generate module`) creates TypeScript interfaces from Go structs. But the event payloads emitted via `runtime.EventsEmit()` are untyped — they're `any` on the TypeScript side. So if you change the shape of `queue.TracksModified` in Go, the `EventsOn` handler in `queue-store.ts` receives the new shape but TypeScript doesn't enforce it. The `applyTracksDelta` method accesses `.action`, `.tracks`, `.index`, `.positions` — if any of these rename, the delta application silently fails (produces `undefined`). +- YellowJacket is currently a fully offline app (INTEGRATIONS.md: "No external API calls, cloud services"). Adding network requests is a new domain with no existing patterns for rate limiting, caching, or error handling. +- MusicBrainz API responses are richly linked — an artist has release groups, each release group has releases, each release has recordings. A naive "fetch everything on click" pattern generates a burst of requests. +- The `inc` parameter in the MusicBrainz API allows requesting related data in a single call (e.g., `?inc=release-groups+recordings`), but many combinations are not allowed together, forcing multiple requests anyway. -**How to avoid:** -1. **After any Go struct change to a type used in events, grep the frontend for all usages of that type's fields.** Event payloads are the blind spot — Wails bindings don't cover them. -2. **Run `wails generate module` after every Go struct change** and check the git diff of the generated TypeScript files. If a field renamed, the diff will show it. -3. **Consider adding a shared event payload validation layer.** The `TracksModified` struct in Go and the `TracksModified` type in `queue-store.ts` must match — add a build step or test that verifies field parity. -4. **Never change JSON tags on event payload structs without updating the TypeScript counterpart.** The JSON tags (`json:"currentIndex"`) are what actually matters for the frontend, not the Go field names. +**Prevention:** +1. **Set a proper User-Agent:** `YellowJacket/ (https://github.com/your/repo)` — this is REQUIRED by MusicBrainz. Without it, the app is rate-limited as "anonymous" (official docs confirm). +2. **Implement a global HTTP rate limiter:** Use `golang.org/x/time/rate` with `rate.NewLimiter(1, 1)` — one request per second, burst of 1. All MusicBrainz API calls go through this limiter. This is the officially documented limit. +3. **Cache aggressively.** MusicBrainz data changes rarely. Cache responses in SQLite (a new `musicbrainz_cache` table with MBID as key, response JSON as value, and a TTL column). Artist data can be cached for days. This eliminates repeat API calls for the same artist/album. +4. **Use `inc` parameters to reduce request count.** Fetch `artist?inc=release-groups` in one call rather than artist + separate release-groups lookup. Check the MusicBrainz API docs for valid `inc` combinations. +5. **Show loading states, not blank pages.** While waiting for rate-limited responses, show skeleton UI with a "Loading from MusicBrainz..." indicator. Queue requests and process them sequentially. +6. **Handle 503 gracefully.** On 503, back off exponentially (2s, 4s, 8s). Show the user "MusicBrainz is rate limiting us, retrying in Xs..." Don't silently fail. -**Warning signs:** -- Queue panel stops updating after a Go struct change -- Event handlers silently receive `undefined` for renamed fields -- `wails dev` works but production build has broken types -- Frontend TypeScript compiles but runtime behavior is wrong +**Detection:** +- Blank artist/album pages in the MusicBrainz browser +- Console shows repeated 503 errors +- All MusicBrainz browsing stops working for ~10 seconds (IP-level block) +- MusicBrainz community reports your app as misbehaving -**Phase to address:** -Every phase that touches Go structs used in events. Add a validation check (build script or test) early. +**Confidence:** HIGH — rate limiting rules confirmed from official MusicBrainz documentation at https://musicbrainz.org/doc/MusicBrainz_API/Rate_Limiting. -## Technical Debt Patterns +--- -| Shortcut | Immediate Benefit | Long-term Cost | When Acceptable | -|----------|-------------------|----------------|-----------------| -| Fixing race without test first | Faster to ship the fix | If fix introduces deadlock or regression, no test catches it. May need to fix again. | Only for trivial races like `SetContext` one-liners where the fix is mechanical (add lock around single assignment) | -| Using `:memory:` SQLite for all tests | Faster tests, no cleanup | Hides WAL behavior, FK enforcement, migration ordering issues | Acceptable for pure query logic tests. Never for integration or migration tests. | -| Keeping raw SQL for batch operations | Avoids sqlc limitations with dynamic IN clauses | Diverges from project's type-safe query pattern. No compile-time checking. | Acceptable when documented. sqlc's `sqlc.slice()` has limitations with SQLite that may not support the batch pattern. | -| Full queue rewrite on every mutation | Simple, always-consistent persistence | O(n) for every single add/remove. For 5000-track queues, this is noticeable. | Acceptable for SetQueue and RestoreState. Not acceptable for AddTrack/RemoveTrack hot paths. | -| Skipping player tests due to hardware | No CI flakiness from audio devices | Player regressions only caught manually. Volume math, state serialization, streamer chain setup are all untested. | Extract pure logic into testable functions. The actual speaker interaction can stay integration-only. | -| `startupErr` as package-level var | Simple error propagation between OnStartup and OnDomReady | Not thread-safe, not testable, global mutable state | Never — move to struct field. Low effort, high correctness gain. | +### Pitfall 6: Smart Playlists Trigger Expensive Full-Table Scans on Every Library Change -## Integration Gotchas +**Feature area:** Smart playlists +**What goes wrong:** A smart playlist with filter rules like "genre = 'Rock' AND year > 2000 AND playCount > 5" must be re-evaluated whenever the library changes (scan complete, tag edit, etc.). If evaluation queries the full `track_metadata` VIEW (which already joins 5 tables) with additional filter conditions, each smart playlist re-evaluation is a full table scan. With 10 smart playlists and a 50k-track library, a library scan completion triggers 10 expensive queries simultaneously, blocking the single SQLite writer for seconds and freezing the UI. -| Integration | Common Mistake | Correct Approach | -|-------------|----------------|------------------| -| beep speaker + Player mutex | Calling `speaker.Lock()` from a code path that already holds `p.mu` in a blocking manner, or removing the goroutine dispatch in the beep callback | Maintain strict ordering: `p.mu` before `speaker.Lock()`. Keep `go p.onPlaybackFinished()` as a goroutine dispatch. Never hold both locks when calling into queue. | -| Wails event system + TypeScript stores | Assuming event delivery order matches emission order. Wails events are async from Go → JS bridge. Two events emitted sequentially in Go may arrive in either order in TS. | Design stores to handle events in any order. Use full-state events (`QueueChanged`) as periodic correction. Don't rely on `QueueTracksModified` always arriving before `QueueIndexChanged`. | -| sqlc + FTS5 virtual tables | Expecting sqlc to generate queries against FTS5 `MATCH` syntax. sqlc's SQLite support doesn't fully understand FTS5 virtual table syntax. | Keep FTS5 queries as hand-crafted SQL. Only use sqlc for standard table queries. Document FTS queries as intentional exceptions to the sqlc pattern. | -| modernc.org/sqlite + PRAGMA | Assuming PRAGMAs persist across connections. With the pure-Go driver, each new connection (from the pool) starts fresh. `SetMaxOpenConns(1)` mitigates this but `foreign_keys` must still be set per connection. | Set `PRAGMA foreign_keys = ON` immediately after opening, as the codebase already does. For tests, replicate this in the test helper. | -| TOML config + new fields | Adding a new config section without a default. Existing users' TOML files don't have the new section. `toml.Decode` leaves it as `nil`. `applyDefaults()` runs after decode but only creates defaults for `nil` sections — doesn't fill in missing fields within existing sections. | Always add defaults in `applyDefaults()` for new fields. Test config loading with an empty file and a minimal file (only `[Library]` section). | -| Wails lifecycle + SetContext ordering | Calling `RestoreState()` before `SetContext()`. The restore tries to emit events but context is nil. Or calling `SetPlayer()` after `RestoreState()` — the restored queue tries to auto-advance but player reference is nil. | Follow the exact ordering in `OnStartup()`: SetContext → SetPlayer → RestoreState. Document this ordering requirement. Test with a mock that verifies call order. | +**Why it happens:** +- The `track_metadata` VIEW (schemas/track_metadata_view.sql) joins `audio_files`, `recordings`, `artist_credit`, `release_group_recordings`, `release_groups`, `genres`, and `file_types`. Adding smart playlist filters on top of this VIEW means SQLite can't use indexes effectively — VIEWs are expanded inline. +- SQLite's `SetMaxOpenConns(1)` means all these queries serialize. Even read queries block behind any pending write. +- Smart playlist re-evaluation is triggered by `LibraryScanComplete` events. If 10 smart playlists each take 200ms to evaluate, that's 2 seconds of blocked database access. -## Performance Traps +**Prevention:** +1. **Don't re-evaluate all smart playlists on every library change.** Instead, mark smart playlists as "stale" when the library changes, and only re-evaluate when the user views the playlist. +2. **Write dedicated sqlc queries for smart playlist evaluation** that target specific indexed columns directly on `audio_files` and `recordings` tables, rather than going through the `track_metadata` VIEW. For example, a "genre = Rock" filter should query `genre_recordings JOIN genres` directly with an index on `genres.name`. +3. **Add indexes for common smart playlist filter columns:** `recordings.year`, `genres.name`, `recordings.name` if not already indexed. Check existing indexes before adding. +4. **Batch evaluation.** If multiple smart playlists need re-evaluation, evaluate them in a single transaction to amortize the transaction overhead. +5. **Store smart playlist rules as JSON in a new `smart_playlists` table**, separate from the existing `playlists` table. Smart playlists don't have fixed track lists — they have rules. Mixing them into the same table complicates the playlist code. +6. **Consider a play_count column.** Smart playlists often filter by play count, but there's no `play_count` column in the current schema. This needs a schema migration adding it to `audio_files` or `recordings`, with an increment trigger on playback completion. -| Trap | Symptoms | Prevention | When It Breaks | -|------|----------|------------|----------------| -| Full queue persistence on every mutation | Slight lag when adding/removing single tracks. `commitMutation()` calls `persistTracks()` which does DELETE + INSERT ALL. | Profile `persistTracks()` for queue sizes of 100, 1000, 5000 tracks. Implement incremental persistence for single-track operations. | Queues > 1000 tracks with frequent mutations (drag-reorder, bulk add). ~50-100ms per operation at 5000 tracks with SQLite writes. | -| Eager full-library fetch on startup | Slow initial load for large libraries. Four simultaneous `GetAll*` queries each doing full table scans with JOINs. | Measure actual query times: if < 300ms for target library size, keep eager. If > 300ms, lazy-load non-default views. | Libraries > 50k tracks. Each `GetAllTracks` query with JOIN chain may take 500ms+. | -| FTS5 JOIN chain in every search query | Search latency scales with library size. The 5-table JOIN chain runs for every keystroke (debounced). | The JOIN chain is necessary for displaying results. Optimize by ensuring FTS5 index is populated correctly so `MATCH` reduces the result set before JOINs. Add `LIMIT` to all search queries. | Libraries > 100k tracks without proper FTS5 indexing. | -| Frontend re-renders on every store notification | Track list with 10k+ items re-renders when any store property changes. Virtual scrolling helps but the data array replacement triggers Lit's dirty check. | Use `===` reference equality checks. Only replace arrays when contents actually changed, not on every event. Lit's `@state()` triggers re-render on any assignment. | Track lists > 5000 items with frequent events (playback position updates). | -| SetQueue Phase 2 re-fetches all tracks | `resolveRemainingTracks` calls `lookupTrackMetaBatch(filePaths)` for ALL paths including those already resolved in Phase 1. | Pass Phase 1 results to Phase 2. Only look up the delta. For a 5000-track album, this saves ~50 lookups. | Large playlists/albums > 500 tracks where Phase 1's 50-track window is a small fraction. | +**Detection:** +- UI freezes for several seconds after library scan completes +- SQLite `busy_timeout` errors in logs during smart playlist evaluation +- Smart playlist contents don't update until app restart (stale evaluation) -## UX Pitfalls +**Confidence:** HIGH — the VIEW structure and single-writer constraint are confirmed from the codebase. The performance concern is proportional to library size. -| Pitfall | User Impact | Better Approach | -|---------|-------------|-----------------| -| Introducing loading states where none existed | User who has been using the app daily suddenly sees spinners or empty states on startup. Perceives app as slower even if total time is the same. | Preserve instant-display for the default view. Only add loading states for lazily-loaded secondary views (artist detail, genre browsing). Use skeleton UIs, not spinners. | -| Fixing queue persistence timing | If incremental persistence introduces a delay between mutation and save, a crash between mutation and save loses the change. User adds 50 tracks, app crashes, queue is reverted. | Persist synchronously for user-initiated mutations (add, remove). Only defer persistence for background operations (Phase 2 resolve). | -| Changing search result ranking | Consolidating FTS5 queries might change which columns are weighted. User's muscle memory for search ("typing 'beat' always shows Beatles first") breaks silently. | Capture current search results for common queries before refactoring. Validate ranking stability after changes. | -| Config migration failures | User's config.toml has custom theme settings. A config change causes parse failure on startup. App doesn't start. User has no way to recover without deleting config. | Always handle TOML parse errors gracefully — log the error, use defaults, don't crash. The current code returns an error from `NewConfig()` which is fatal. Consider falling back to defaults with a warning. | -| Event ordering changes | Refactoring changes when events are emitted relative to state changes. Frontend shows stale data for a frame (queue shows old index while track changed). | Ensure state is consistent before emitting any events. Emit all related events together. Use the full-state `QueueChanged` event as the ground truth; deltas are optimizations. | +--- -## "Looks Done But Isn't" Checklist +### Pitfall 7: Keyboard Shortcut System Conflicts with Browser/WebView Defaults and Shadow DOM -- [ ] **Queue tests:** Often missing concurrent SetQueue test — verify two rapid SetQueue calls don't corrupt state (generation counter works) -- [ ] **Search consolidation:** Often missing empty-string and special-character test cases for FTS5 — verify `"`, `*`, `(`, `)` in search queries don't crash -- [ ] **Config roundtrip:** Often missing test with unknown TOML keys — verify future config fields don't cause parse errors on older app versions -- [ ] **Migration tests:** Often missing test on existing database with data — verify migration doesn't drop existing rows -- [ ] **Incremental persistence:** Often missing test for queue order after remove-from-middle — verify remaining tracks keep correct positions -- [ ] **Lock ordering:** Often missing test for rapid LoadFile during playback — verify the beep callback + new LoadFile don't deadlock -- [ ] **Event parity:** Often missing validation that Go event constants match TypeScript — verify no typos exist between `events.go` and `events.ts` -- [ ] **Lazy loading:** Often missing test for component render with null data — verify all components handle loading state without errors -- [ ] **FTS rebuild:** Often missing test for `RebuildSearchIndex` idempotency — verify running it twice doesn't create duplicate index entries +**Feature area:** Customizable keyboard shortcuts +**What goes wrong:** The user configures Ctrl+L as "next track," but WebKitGTK intercepts Ctrl+L as "focus address bar" (or similar browser-internal shortcut). Or the user maps Space to "play/pause," but pressing Space while focused on a button triggers the button's click handler AND the global shortcut. Shadow DOM boundaries in Lit components further complicate event propagation — a keyboard event inside a component's shadow root may not bubble to the document-level shortcut handler. -## Recovery Strategies +**Why it happens:** +- Wails v2 uses a native WebView (WebKitGTK on Linux). The WebView has its own keyboard shortcut handling that runs before JavaScript event handlers. Some key combinations are intercepted before they reach the page. +- The existing Ctrl+F handler in `index.ts` (line 157) uses `document.addEventListener('keydown', ...)`. This works because it's at the document level. But components with shadow DOM (all Lit components in this project) create isolated event boundaries. A `keydown` event on an `` inside a shadow root does bubble to the document, but `event.composedPath()` must be used to determine the actual target. +- Keyboard shortcuts that overlap with form controls (Space, Enter, arrow keys, Tab) interfere with normal text input, button interaction, and accessibility navigation. -| Pitfall | Recovery Cost | Recovery Steps | -|---------|---------------|----------------| -| Deadlock from lock ordering violation | LOW | Identify the two goroutines holding locks (SIGQUIT dump). Fix the ordering. Add a comment. The app just needs restart — no data loss. | -| Silent search regression from FTS consolidation | MEDIUM | Revert the consolidation. Write the tests that should have existed. Re-apply consolidation with tests passing. Data is intact — only query logic changed. | -| Queue state loss from persistence change | HIGH | If queue_tracks table was corrupted, user loses their queue. No automatic recovery. Prevention: always write persistence tests before changing the write path. Mitigation: keep a backup of queue state in a second table during migration period. | -| Config parse failure on startup | MEDIUM | App won't start. User must manually edit or delete config.toml. Prevention: handle TOML errors gracefully, fall back to defaults. Recovery: add a `--reset-config` CLI flag. | -| Frontend empty state regressions | LOW | Components show blank instead of data. Fix by adding null checks and loading states. No data loss. But user trust is eroded. | -| Wails binding mismatch after struct rename | MEDIUM | Frontend silently receives undefined fields. Fix by running `wails generate module` and updating TypeScript event handlers. No data loss but broken UI until fixed. | -| In-memory test false positive | HIGH (delayed) | Tests pass, bug ships. Discovered when user reports data loss or corruption in production. Prevention: use file-based SQLite in tests from the start. Recovery depends on which bug shipped. | +**Prevention:** +1. **Use `document.addEventListener('keydown', ..., { capture: true })` for global shortcuts.** The capture phase fires before any component-level handlers can `stopPropagation()`. This is where the shortcut system should live. +2. **Skip shortcuts when focus is on an input/textarea.** Check `document.activeElement` (and use `event.composedPath()` to see through shadow DOM) — if the focused element is an input, text area, or contenteditable, don't handle the shortcut unless it uses a modifier key (Ctrl, Alt, Meta). +3. **Maintain a conflict list of reserved key combinations.** Some keys cannot be remapped because WebKitGTK intercepts them: Ctrl+C/V/X (copy/paste/cut), Ctrl+A (select all), Tab (focus navigation). Document these as non-configurable. +4. **Store shortcuts in the TOML config** under a `[Shortcuts]` section. Use the existing config event pattern — `ShortcutsConfigChanged` event triggers frontend re-registration. Don't store shortcuts in the frontend — the backend is the source of truth. +5. **Use the `key` property, not `keyCode`.** `keyCode` is deprecated and varies by keyboard layout. `event.key` is layout-aware and returns "a" regardless of whether the user has QWERTY or AZERTY. -## Pitfall-to-Phase Mapping +**Detection:** +- Some key combinations "don't work" on Linux but work on macOS (WebView intercepts differently) +- Typing in a search or playlist name triggers shortcut actions +- Shortcut works when focus is on the track list but not when focus is inside a shadow DOM component -| Pitfall | Prevention Phase | Verification | -|---------|------------------|--------------| -| Refactoring concurrency without tests | Testing infrastructure (first phase) | Queue characterization tests pass. `-race` flag clean on all test runs. | -| In-memory SQLite test divergence | Testing infrastructure (first phase) | `NewTestDB()` helper uses file-based SQLite with identical pragma setup. All DB tests use it. | -| Player deadlock from lock ordering | Player refactoring phase (after testing) | Pure logic extracted and tested. Lock-sensitive code unchanged or minimally changed with lock graph documented. No SIGQUIT needed. | -| FTS5 query consolidation breaks search | Database/code quality phase | Search tests capture before/after results for: full metadata track, metadata-less track, special characters, empty query. Zero-diff after consolidation. | -| Eager-to-lazy loading UX regression | Performance phase | Profile data establishes baseline. If lazy loading applied, all `getCached*()` call sites handle null. Skeleton UI visible for < 200ms. | -| Queue persistence state loss | Performance phase | Queue persistence roundtrip tests pass. Old-format → new-format compatibility test passes. Queue survives app restart in all modes. | -| Wails binding mismatch | Every phase (continuous) | `wails generate module` runs in CI or pre-commit. Event payload types have TypeScript interface definitions that match Go struct JSON tags. | -| Config migration failure | Correctness phase | Config roundtrip test with empty file, minimal file, and full file. Unknown keys don't crash. Missing sections get defaults. | -| Event ordering assumptions | Correctness/UX phase | Frontend stores handle events in any order. Full-state events correct drift. No visible flicker between events. | +**Confidence:** HIGH — the shadow DOM event boundary behavior is fundamental to Lit/Web Components. WebView keyboard interception is platform-specific and confirmed by Wails community reports. + +--- + +### Pitfall 8: Layout Customization Breaks Component Assumptions About Size and Context + +**Feature area:** Layout customization system +**What goes wrong:** The current layout is hardcoded in `index.html` with a CSS Grid template: `"top-bar top-bar" 4em "sidebar main-panel" 1fr "bottom-bar bottom-bar" 4em`. Components assume their grid area and available space — `track-list` expects to fill the main panel, `now-playing` expects to be in the bottom bar with exactly 4em height. When users can rearrange sections, a component designed for a wide horizontal area (queue panel) gets placed in a narrow sidebar slot, or the `audio-player` component that assumes bottom-bar positioning gets placed in the sidebar where its progress bar layout breaks. + +**Why it happens:** +- Components use CSS that assumes their container context. For example, `now-playing` uses `grid-template-columns: var(--now-playing-width, 200px) 1fr auto` in the bottom bar (index.css line 68). Moving it elsewhere breaks this layout. +- The `@lit-labs/virtualizer` used for large lists requires a fixed-height container to calculate visible items. If the track list is placed in a container without explicit height, virtual scrolling breaks — it either renders all items (defeating the purpose) or renders none. +- Navigation routing in `index.ts` uses `document.getElementById('main-content')` and replaces its `innerHTML`. Layout customization means there might be multiple content areas or the main content area might have a different ID. + +**Prevention:** +1. **Components must declare size constraints.** Define a component metadata interface: `{ minWidth: number, minHeight: number, resizable: boolean, preferredArea: 'main' | 'sidebar' | 'footer' | 'any' }`. The layout system validates placements against constraints. +2. **Use CSS Container Queries for responsive components.** Instead of assuming "I'm in the sidebar" or "I'm in the main panel," components should use `@container` queries to adapt their layout based on available space. This requires adding `container-type: inline-size` to layout section containers. +3. **The layout system should operate at the section level, not the component level.** Sections have fixed roles (navigation, content, playback controls, queue). Users configure which components appear in each section and section sizes, but the section structure itself remains constrained. This is the MusicBee model. +4. **Don't refactor existing components for layout flexibility in the first pass.** Instead, build the layout configuration system that works with the current component set. Mark certain components as "fixed position" (audio-player must be in footer, sidebar must exist). Allow the content area to swap between different content components. Expand flexibility in later iterations. +5. **Virtual scrolling containers need explicit height.** Any section that hosts a virtualized list must provide a concrete CSS height (not `auto`). The layout system must enforce this for sections marked as "supports-virtualization." + +**Detection:** +- Virtual scrolling breaks when components are moved to different sections +- Components render with broken layouts (overlapping, zero height, horizontal overflow) +- Navigation stops working because `main-content` element doesn't exist in the new layout + +**Confidence:** HIGH — the hardcoded grid layout and component CSS assumptions are confirmed from index.html and index.css analysis. + +--- + +### Pitfall 9: Tag Editing and Library Scan Compete for SQLite Writer and File Access + +**Feature area:** Tag editing + library scanning interaction +**What goes wrong:** The user edits a track's tags while a library scan is in progress. The tag write modifies the file on disk, then tries to update the database. Simultaneously, the scan's DB writer goroutine is batching inserts in a transaction. The tag edit's UPDATE waits on `busy_timeout` (5000ms). Meanwhile, the scan discovers the same file during its walk — the file's modification time has changed (because of the tag write), so the scan processes it again, overwriting the just-saved tag edits with the data it reads from the file. But the file now has the NEW tags, so the scan reads the new data... unless the scan started reading the file before the tag write completed, in which case it reads a partially written file and gets corrupted metadata. + +**Why it happens:** +- SQLite single-writer with WAL mode allows concurrent reads, but writes serialize. The scan's batch transaction holds the writer lock for the duration of each batch (50 files). A tag edit UPDATE must wait for the batch to commit. +- The scan pipeline's Phase 2 (walk) checks file existence and mod time against the `sync.Map` of existing files. If a tag write changes the file between the `sync.Map` population (Phase 1) and the walk (Phase 2), the file appears "modified" and gets reprocessed. +- The metadata extraction worker pool (Phase 3) reads the file concurrently with the user's tag write. There's no file-level locking. + +**Prevention:** +1. **Block tag editing during active scans.** The simplest and most robust approach. Check `l.scanning` (add an atomic bool) before allowing tag writes. Return a user-friendly error: "Cannot edit tags while library scan is in progress." +2. **Alternatively, use a per-file advisory lock.** Before writing tags, acquire an in-memory lock for that file path. Before the scan processes a file, check the same lock. This is more complex but allows tag editing during scans for non-conflicting files. +3. **After a tag write, mark the file as "recently edited" with a timestamp.** The scan's walk phase should skip files edited within the last N seconds to avoid re-processing files that were just intentionally modified. +4. **The tag write endpoint should be a single Go method that coordinates all steps atomically:** stop playback if needed → write temp file → rename → update database → update FTS5 → emit events. Don't let the caller orchestrate these steps. + +**Detection:** +- Tag edits "revert" after a library scan completes +- `SQLITE_BUSY` errors in the tag edit path during scans +- Corrupted metadata for files that were edited during a scan + +**Confidence:** HIGH — the single-writer constraint and scan pipeline concurrency model are confirmed from the codebase. + +--- + +## Minor Pitfalls + +These cause developer frustration or minor user issues but are containable. + +--- + +### Pitfall 10: MusicBrainz Data Model Mismatch with YellowJacket Schema + +**Feature area:** MusicBrainz browser +**What goes wrong:** MusicBrainz uses a different data model than YellowJacket's schema. MusicBrainz has release groups (albums), releases (specific editions), and recordings (tracks). YellowJacket's schema already mirrors some of this (tables named `release_groups`, `recordings`, `artist_credit`), but the mapping isn't perfect — YellowJacket's `release_groups` are "albums" with a single name, while MusicBrainz release groups have types (Album, Single, EP, Compilation), dates, and disambiguation comments. Trying to merge MusicBrainz browsing data into the existing schema creates confusion about which data is "local library" and which is "MusicBrainz catalog." + +**Prevention:** +1. **Keep MusicBrainz browser data completely separate from the library database.** Use a separate cache table (`musicbrainz_cache`) or even an in-memory map. The MusicBrainz browser is read-only catalog browsing — it shouldn't modify library data. +2. **Map MusicBrainz entities to display-only DTOs**, not to the existing sqlcgen types. Create separate TypeScript interfaces (`MBArtist`, `MBReleaseGroup`, `MBRecording`) that the MusicBrainz browser components consume. +3. **If linking local tracks to MusicBrainz IDs (for future features like automatic tagging), store MBIDs as optional columns** on existing tables (e.g., `recordings.musicbrainz_id TEXT`), not as foreign keys to MusicBrainz tables. This is a one-way link — local data points to MusicBrainz, not the reverse. + +**Confidence:** MEDIUM — the schema naming overlap is confirmed, but the exact API response structure would need to be verified against the MusicBrainz API at implementation time. + +--- + +### Pitfall 11: Crossfade Sample-Rate Mismatch Between Outgoing and Incoming Tracks + +**Feature area:** Gapless playback + crossfade +**What goes wrong:** Track A is a 44.1kHz MP3 and Track B is a 96kHz FLAC. Both are resampled to the speaker rate (44100Hz), but the resampling happens in `updateStreamers()` which creates a new resample chain for each track. During crossfade, both tracks must produce samples at the same rate for mixing. If the crossfade streamer reads raw samples from pre-resample streamers, the mix produces garbage audio (different sample rates interpreted as the same). + +**Prevention:** +1. **Always crossfade post-resample.** The crossfade mixer must receive samples that are already resampled to the speaker rate. Since `updateStreamers()` already handles resampling, ensure the crossfade operates on the resampled output, not the raw decoder output. +2. **The crossfade streamer should accept two `beep.Streamer` interfaces** (not `beep.StreamSeeker`), because the resampled streamers don't support seeking. This matches beep's design where resampled streamers lose the StreamSeeker interface. + +**Confidence:** HIGH — the resample chain is confirmed in player.go lines 309-313. The speaker rate is hardcoded to 44100. + +--- + +### Pitfall 12: Config TOML Backward Compatibility When Adding New Sections + +**Feature area:** Keyboard shortcuts, layout customization +**What goes wrong:** Adding `[Shortcuts]` and `[Layout]` sections to config.toml works for new installations (defaults applied), but existing users have config files without these sections. The TOML decoder fills in zero values for missing sections. If the code checks `config.Shortcuts != nil` but TOML decoding creates an empty struct (not nil), the nil check passes but the struct has zero-value fields. The `applyDefaults()` function runs before decode (see CONCERNS.md line 168: "applyDefaults runs after decode which could overwrite valid zero values"), creating a timing issue. + +**Prevention:** +1. **Follow the existing pattern:** `applyDefaults()` sets defaults, then TOML `Decode()` overwrites with user values. New sections get populated defaults even if the user's file doesn't contain them. This already works correctly for existing sections. +2. **Add defaults for ALL new fields in `applyDefaults()`.** For shortcuts, provide a complete default keybinding map. For layout, provide the default layout matching the current hardcoded grid. +3. **Test with an empty config file and an old-format config file.** The `config_test.go` should verify that loading a TOML file without `[Shortcuts]` or `[Layout]` produces valid defaults. +4. **Never use nil checks for TOML-decoded sections.** The TOML decoder creates zero-value structs, not nil pointers. Use a validation method that checks for meaningful content (e.g., "shortcuts map is empty" not "shortcuts is nil"). + +**Confidence:** HIGH — the config loading pattern is confirmed from config.go and CONCERNS.md. + +--- + +### Pitfall 13: Wails Event Bridge Payload Size for MusicBrainz and Layout Data + +**Feature area:** MusicBrainz browser, layout customization +**What goes wrong:** The Wails event system serializes payloads as JSON through the WebView bridge. A MusicBrainz artist response with full discography (release groups, releases with track listings) can be 100KB+ of JSON. Emitting this via `runtime.EventsEmit()` means serializing to JSON in Go, passing through the WebView bridge, and deserializing in JavaScript. For large payloads, this introduces noticeable latency. Similarly, saving/loading a complex layout configuration with per-component state creates large event payloads. + +**Prevention:** +1. **Use Wails function bindings (direct calls) for large data transfers, not events.** Events are for notifications ("data changed"). Bindings are for data retrieval ("give me the data"). The frontend should call a Go binding method that returns the MusicBrainz data directly, not listen for an event with the data embedded. +2. **Paginate MusicBrainz results.** Don't load an artist's entire discography at once. Load release groups first (lightweight), then load releases for a specific release group on click (lazy loading). +3. **For layout config, store in the TOML file and load via the existing config binding pattern.** Don't emit the full layout through events — load it once at startup via `Config.GetLayoutConfig()` binding. + +**Confidence:** MEDIUM — Wails event serialization overhead depends on WebView implementation. The recommendation to use bindings over events for data is based on Wails architecture best practices. + +--- + +### Pitfall 14: Frontend Store Proliferation and Controller Explosion + +**Feature area:** Smart playlists, MusicBrainz browser, layout customization, plugin system +**What goes wrong:** Each new feature area gets its own store and controller: `SmartPlaylistStore + SmartPlaylistController`, `MusicBrainzStore + MusicBrainzController`, `LayoutStore + LayoutController`, `PluginStore + PluginController`, `ShortcutStore + ShortcutController`. The project goes from 8 store/controller pairs to 13+. Each pair requires: a singleton store class, event subscriptions, a controller class with `hostConnected`/`hostDisconnected`, barrel file exports, and event name constants in both Go and TypeScript. The boilerplate adds up and the store/controller pattern becomes a maintenance burden. + +**Prevention:** +1. **Not every feature needs its own store.** MusicBrainz data is view-local (only relevant when the user is browsing MusicBrainz) — it can live as component-local state in the MusicBrainz browser component, not a global store. +2. **Smart playlist rules are part of playlist data** — extend the existing `PlaylistStore` rather than creating a new store. +3. **Keyboard shortcuts and layout config are extensions of the existing config system.** Extend `Config` (backend) and load via the existing config binding. The frontend reads once at startup; changes are rare. +4. **Only create a new store when the data is: (a) shared across multiple components, (b) updated from backend events, AND (c) needed across different views.** If data is view-local or rarely changes, use component state or a simple module-level variable. + +**Confidence:** HIGH — the store/controller pattern is confirmed from the codebase. The frontend already has 8 stores for ~15 components. + +--- + +### Pitfall 15: Event Name Constants Drift with Many New Events + +**Feature area:** All features (cross-cutting) +**What goes wrong:** Adding tag editing, scan cancellation, smart playlists, MusicBrainz, shortcuts, layout, and plugins requires ~15-20 new event names. Each must be added to both `backend/events/events.go` and `frontend/src/events.ts`. The AST-based codegen (`genevents`) generates TypeScript from Go, but only if you run `go generate`. Forgetting to regenerate after adding an event in Go leaves the TypeScript file stale. The pre-commit hook checks for codegen freshness, but a developer working in the frontend first (adding a TypeScript event) has no corresponding Go constant. + +**Prevention:** +1. **Always add events in Go first.** The codegen flows Go → TypeScript. Never add events in TypeScript manually. This is already documented but worth reinforcing with 15+ new events being added. +2. **Run `make generate` as part of the development workflow,** not just before commit. The pre-commit hook is a safety net, not the primary mechanism. +3. **Group new events by feature area** in `events.go` with section comments, matching the existing pattern (Playback, Queue, Config, Playlist, Library). Add new groups: `Tag`, `SmartPlaylist`, `MusicBrainz`, `Layout`, `Plugin`, `Shortcuts`, `Scan`. +4. **Consider adding a build-time check** that counts events in both files and fails if they differ. The current codegen check verifies file freshness but not content correctness if someone manually edited the TypeScript. + +**Confidence:** HIGH — the codegen pattern and its fragility are documented in CONCERNS.md. + +--- + +## Phase-Specific Warnings + +| Phase Topic | Likely Pitfall | Mitigation | +|-------------|---------------|------------| +| Tag editing | File corruption during write (P1) | Write-to-temp-then-rename; block writes on playing file | +| Tag editing | SQLite contention with scan (P9) | Block tag edits during active scans | +| Scan cancellation | Orphan cleanup on partial scan (P3) | Skip orphan cleanup when cancelled | +| Scan cancellation | Goroutine leaks on cancel (P3) | Drain all channels; use scan-specific context | +| Gapless playback | Lock ordering deadlock (P2) | Pre-decode in separate goroutine; suppress callback during transition | +| Crossfade | Sample rate mismatch (P11) | Always crossfade post-resample streamers | +| Smart playlists | Full-table scans (P6) | Lazy evaluation; dedicated indexed queries | +| Smart playlists | Missing play_count column (P6) | Schema migration with increment on playback | +| MusicBrainz browser | Rate limiting (P5) | 1 req/s rate limiter; aggressive caching; proper User-Agent | +| MusicBrainz browser | Schema confusion (P10) | Separate cache table; display-only DTOs | +| Keyboard shortcuts | Shadow DOM event boundaries (P7) | Capture phase listener; composedPath() for target detection | +| Keyboard shortcuts | WebView key interception (P7) | Document reserved keys; skip shortcuts on input focus | +| Layout customization | Component size assumptions (P8) | Container queries; component size constraints metadata | +| Layout customization | Virtual scrolling breakage (P8) | Explicit height enforcement for virtualized sections | +| Plugin system | Host crash from plugin panic (P4) | Embedded scripting runtime (not native Go plugins) | +| Plugin system | SQLite contention (P4) | Read-only connection for plugins; namespaced events | +| Config additions | Backward compatibility (P12) | Defaults for all new fields; test with old config files | +| All features | Event name drift (P15) | Go-first workflow; grouped event constants; codegen validation | +| All features | Store/controller proliferation (P14) | Extend existing stores; use component-local state where appropriate | +| MusicBrainz + Layout | Event payload size (P13) | Use bindings for data; events for notifications only | + +--- + +## Feature Interaction Matrix + +Some pitfalls emerge from the interaction between features, not from individual features: + +| Feature A | Feature B | Interaction Pitfall | +|-----------|-----------|-------------------| +| Tag editing | Library scan | Writer contention + file access races (P9) | +| Tag editing | Gapless playback | Can't write tags on file being played or pre-decoded (P1) | +| Smart playlists | Tag editing | Smart playlists must re-evaluate after tag edits change matching criteria | +| Smart playlists | Library scan | Smart playlists must re-evaluate after scan adds/removes tracks (P6) | +| Gapless playback | Plugin system | Plugins must not interfere with speaker lock during transitions (P4 + P2) | +| Layout customization | Plugin system | Plugins may want to register custom layout sections — layout system must be extensible | +| Keyboard shortcuts | Plugin system | Plugins may want to register custom shortcuts — shortcut system must be extensible | +| MusicBrainz browser | Tag editing | Future feature: apply MusicBrainz metadata to local files (tag write from MB data) | + +--- ## Sources -- Codebase analysis: `backend/player/player.go` (lock ordering, lines 30-40, 340-394) -- Codebase analysis: `backend/queue/queue.go` (SetQueue two-phase, lines 152-311) -- Codebase analysis: `backend/queue/persistence.go` (full rewrite pattern, lines 116-204) -- Codebase analysis: `backend/database/database.go` (pragma setup, lines 49-65; migrations, lines 153-335) -- Codebase analysis: `backend/database/search.go` (duplicated FTS5 JOINs, lines 34-58, 92-116) -- Codebase analysis: `frontend/src/store/library-store.ts` (eager fetch, lines 300-305; lazy accessors, lines 64-154) -- Codebase analysis: `frontend/src/store/queue-store.ts` (delta application, lines 107-171) -- Codebase analysis: `backend/config/config.go` (load/save roundtrip, lines 100-139, 142-160) -- Codebase analysis: `backend/app.go` (lifecycle ordering, lines 136-212; package-level startupErr, line 134) -- Documented concerns: `.planning/codebase/CONCERNS.md` (all sections) -- Go testing best practices: `t.TempDir()` for file-based test databases (enforced by usetesting linter) -- SQLite documentation: PRAGMA scoping, WAL mode behavior, FTS5 ranking (HIGH confidence — well-established SQLite behavior) -- beep library: speaker lock semantics (HIGH confidence — observed in codebase, consistent with beep v2 design) -- Wails v2: binding generation, event system limitations (MEDIUM confidence — based on codebase patterns and Wails v2 documented behavior) +- **Codebase analysis:** `backend/player/player.go` (lock ordering, callback pattern, BufferedStreamer), `backend/library/library.go` (scan pipeline phases, context cancellation), `backend/database/` (schema, single-writer), `frontend/index.ts` (keyboard handling, navigation), `frontend/index.html` + `index.css` (hardcoded grid layout) +- **MusicBrainz rate limiting:** https://musicbrainz.org/doc/MusicBrainz_API/Rate_Limiting — confirmed 1 req/s per IP, User-Agent requirement, 503 on violation +- **id3v2 Go library:** https://github.com/n10v/id3v2 — 359 stars, supports ID3v2.3/v2.4 read/write, last release v2.1.4 (Feb 2023) +- **beep wiki (composing/controlling):** https://github.com/gopxl/beep/wiki/Composing-and-controlling — confirmed speaker.Lock() usage, beep.Seq for chaining, beep.Ctrl for pause, effects.Volume for volume control +- **Project context:** `.planning/PROJECT.md`, `.planning/codebase/ARCHITECTURE.md`, `.planning/codebase/CONCERNS.md`, `.planning/codebase/INTEGRATIONS.md` --- -*Pitfalls research for: YellowJacket consolidation milestone* -*Researched: 2026-02-27* + +*Pitfalls research: 2026-03-06* diff --git a/.planning/research/STACK.md b/.planning/research/STACK.md index a35650c..dd7e012 100644 --- a/.planning/research/STACK.md +++ b/.planning/research/STACK.md @@ -1,657 +1,511 @@ -# Stack Research: Consolidation Patterns & Tools +# Technology Stack Additions: v1.1 Features & Extensibility -**Domain:** Desktop music player consolidation — correctness, performance, testing, code quality -**Researched:** 2026-02-27 -**Confidence:** HIGH (core Go/SQLite patterns) / MEDIUM (beep-specific, Lit optimization) +**Project:** YellowJacket v1.1 +**Researched:** 2026-03-06 +**Overall confidence:** HIGH (tag writing, beep audio) / MEDIUM (MusicBrainz API, plugin architecture) -This document covers tools, patterns, and specific techniques for improving the quality of the existing YellowJacket codebase. It is organized by the five research questions, prioritized by impact. +This document covers **only new libraries and patterns** needed for v1.1 features. The existing stack (Go 1.25, Wails v2.10.2, Lit 3.2.1, beep v2.1.1, modernc.org/sqlite, dhowden/tag, BurntSushi/toml, etc.) is validated and unchanged. --- -## 1. Go Concurrency Safety — Priority: CRITICAL +## Recommended Stack Additions -**Confidence:** HIGH — based on Go standard library docs, race detector behavior, and codebase analysis. +### 1. Tag Writing — MP3 (ID3v2) -### The Core Problem +| Technology | Version | Import Path | Purpose | Why | +|------------|---------|-------------|---------|-----| +| n10v/id3v2 | v2.1.4 | `github.com/n10v/id3v2/v2` | Read/write ID3v2.3 and v2.4 tags for MP3 files | The only maintained pure-Go library with full ID3v2 write support. 359 stars, 43 releases, active (last release Feb 2023, stable). dhowden/tag (existing) is read-only — it cannot write tags back. | -YellowJacket has three documented data races, all following the same anti-pattern: a `SetContext()` method writes a struct field without holding the struct's mutex, while other methods read that field under the mutex. This is a textbook data race even if "it works in practice." - -### Pattern: Fix SetContext Races - -The `Queue.SetContext()`, `Library.SetContext()`, and `playlist.Service.SetContext()` all share the same bug. The fix is the same for all three: +**Confidence:** HIGH — verified via GitHub repo, pkg.go.dev. The v2 module path uses the `/v2` subdirectory pattern (`github.com/n10v/id3v2/v2`). +**API surface used:** ```go -// BEFORE (race): -func (q *Queue) SetContext(ctx context.Context) { - q.ctx = ctx // ← no lock, but q.ctx is read under q.mu elsewhere +tag, err := id3v2.Open("file.mp3", id3v2.Options{Parse: true}) +defer tag.Close() +tag.SetArtist("New Artist") +tag.SetTitle("New Title") +tag.SetAlbum("New Album") +tag.SetGenre("Electronic") +tag.SetYear("2024") +// Write back to file +err = tag.Save() +``` + +**Integration notes:** +- Operates on the file directly (open, modify, save). Does not need the existing beep pipeline. +- Must close the file before beep can play it — coordinate with player via a "stop playback → write tags → reload" flow. +- Keep `dhowden/tag` for reading (scan pipeline uses it). Use `n10v/id3v2` only for writing MP3 files. +- Thread safety: id3v2 file operations are not concurrent-safe. The tag editor backend service should serialize writes. + +### 2. Tag Writing — FLAC (Vorbis Comments) + +| Technology | Version | Import Path | Purpose | Why | +|------------|---------|-------------|---------|-----| +| go-flac/go-flac | v2.x | `github.com/go-flac/go-flac/v2` | Parse and reassemble FLAC file metadata blocks | Low-level FLAC metadata manipulation. 44 stars. Provides `ParseFile`, modify `Meta` slice, `Save`. | +| go-flac/flacvorbis | v2.x | `github.com/go-flac/flacvorbis/v2` | Read/write Vorbis comment metadata blocks within FLAC files | Companion to go-flac. Provides `ParseFromMetaDataBlock`, `Add`, `Set` for FLAC vorbis comments. 11 stars, but the only option in the Go ecosystem. | + +**Confidence:** MEDIUM — both libraries are small and niche but are the standard Go solution for FLAC tag writing. v2 modules exist in `/v2` subdirectories. + +**API surface used:** +```go +f, err := flac.ParseFile("file.flac") +// Find existing vorbis comment block +var cmt *flacvorbis.MetadataBlockVorbisComment +var cmtIdx int +for idx, meta := range f.Meta { + if meta.Type == flac.VorbisComment { + cmt, _ = flacvorbis.ParseFromMetaDataBlock(*meta) + cmtIdx = idx + } +} +if cmt == nil { + cmt = flacvorbis.New() +} +cmt.Add(flacvorbis.FIELD_TITLE, []byte("New Title")) +cmt.Add(flacvorbis.FIELD_ARTIST, []byte("New Artist")) +cmtMeta := cmt.Marshal() +if cmtIdx > 0 { + f.Meta[cmtIdx] = &cmtMeta +} else { + f.Meta = append(f.Meta, &cmtMeta) +} +f.Save("file.flac") +``` + +**Integration notes:** +- go-flac reads the entire FLAC file into memory (metadata + audio frames). For large FLAC files (100MB+), this uses significant memory. The write operation is atomic (writes full file). +- Same coordination needed: stop playback → write → reload. + +### 3. Tag Writing — OGG Vorbis and WAV + +| Format | Approach | Why | +|--------|----------|-----| +| OGG Vorbis | Defer to v1.2 or use external tool | No mature pure-Go library exists for writing OGG Vorbis comments. The OGG container format makes in-place tag editing complex. Consider shelling out to `vorbiscomment` CLI tool if needed, or defer. | +| WAV | Not needed for v1.1 | WAV files rarely have meaningful tags (no standard tagging convention). INFO chunks exist but are rarely used in music libraries. | + +**Confidence:** HIGH — exhaustive search found no viable pure-Go OGG Vorbis tag writer. + +**Recommendation:** Implement tag editing for MP3 and FLAC first (covers ~95% of music libraries). Show "read-only" indicator for OGG/WAV files in the tag editor UI. Add OGG support later if demand exists. + +### 4. MusicBrainz API Client + +| Technology | Version | Import Path | Purpose | Why | +|------------|---------|-------------|---------|-----| +| **Direct HTTP + encoding/json** | stdlib | — | Query MusicBrainz REST API (JSON format) | Use Go's standard library rather than a third-party client. See rationale below. | + +**Confidence:** HIGH — MusicBrainz API is well-documented REST/JSON. The API is simple enough that a custom thin client is better than available libraries. + +**Why NOT use `michiwend/gomusicbrainz`:** +- Last meaningful commit was years ago, no Go modules support initially (added by community), uses XML parsing. 64 stars but effectively unmaintained. +- The library only supports search and lookup — no browse requests. +- MusicBrainz API supports JSON natively (`fmt=json` or `Accept: application/json`), making XML parsing unnecessary. + +**Why NOT use `go.uploadedlobster.com/musicbrainzws2`:** +- Hosted on SourceHut, harder to verify maintenance status. +- Low adoption (not visible on GitHub). + +**Custom client approach (recommended):** +```go +// backend/musicbrainz/client.go +package musicbrainz + +type Client struct { + httpClient *http.Client + baseURL string + userAgent string + rateLimiter *time.Ticker // MusicBrainz requires max 1 req/sec } -// AFTER (correct): -func (q *Queue) SetContext(ctx context.Context) { - q.mu.Lock() - defer q.mu.Unlock() - q.ctx = ctx +func NewClient(appName, appVersion, contactURL string) *Client { + return &Client{ + httpClient: &http.Client{Timeout: 10 * time.Second}, + baseURL: "https://musicbrainz.org/ws/2", + userAgent: fmt.Sprintf("%s/%s (%s)", appName, appVersion, contactURL), + rateLimiter: time.NewTicker(time.Second), // 1 request per second + } } ``` -**Why this matters:** The Go race detector (`-race` flag) will flag this in tests. Since `make test` already runs with `-race`, any test that exercises `SetContext` alongside event emission will fail. Fixing these races unblocks writing tests for queue, library, and playlist packages. +**MusicBrainz API integration points:** +- **Rate limiting:** MANDATORY — max 1 request per second. Use `time.Ticker` with channel-based throttling. +- **User-Agent:** MANDATORY — must include app name, version, and contact URL. MusicBrainz blocks requests without meaningful user-agents. +- **Endpoints needed for read-only browser:** + - `GET /ws/2/artist/?inc=release-groups&fmt=json` — Artist lookup with discography + - `GET /ws/2/release-group/?inc=releases&fmt=json` — Album editions + - `GET /ws/2/release/?inc=recordings+media&fmt=json` — Track listings + - `GET /ws/2/artist?query=&fmt=json` — Artist search + - `GET /ws/2/release-group?query=&fmt=json` — Album search +- **Response caching:** Cache API responses in SQLite with TTL (e.g., 7 days). MusicBrainz data is slow-changing. Reduces API calls and improves UI responsiveness. +- **No authentication needed:** Read-only lookups and searches are unauthenticated. -**Why not use `sync/atomic`:** `context.Context` is an interface (two words: type pointer + data pointer). `sync/atomic` only works on single-word types. Use the existing mutex. +### 5. Gapless Playback + Crossfade -### Pattern: Player Double-Lock Fix +| Technology | Version | Purpose | Why | +|------------|---------|---------|-----| +| **beep.Mixer** | v2.1.1 (existing) | Mix two streams for crossfade | Already in the dependency tree. `beep.Mixer` dynamically adds/removes streamers and mixes them. `KeepAlive(true)` keeps it playing silence when no streamers are active. | +| **beep.Seq** | v2.1.1 (existing) | Chain streams for gapless | Already used in `startPaused()`. `beep.Seq(s1, s2)` plays s1 then s2 without gap. | +| **effects.Volume** | v2.1.1 (existing) | Per-stream volume for fade curves | Already used for main volume. Create separate Volume wrappers for fade-in/fade-out. | -The player's `SetContext` acquires and releases the mutex twice in succession: +**Confidence:** HIGH — all primitives already exist in beep v2.1.1. +**No new dependencies needed.** Gapless and crossfade are implemented by changing how the streamer chain is composed, not by adding new libraries. + +**Gapless architecture:** ```go -// BEFORE (window between locks): -func (p *Player) SetContext(ctx context.Context) { - p.mu.Lock() - p.ctx = ctx - p.mu.Unlock() +// Instead of: speaker.Play(beep.Seq(currentStream, beep.Callback(onFinished))) +// Use: pre-decode next track and Seq them together. + +// When current track nears end (e.g., 2 seconds remaining): +nextStreamer, nextFormat := decodeNextTrack() +resampled := beep.Resample(4, nextFormat.SampleRate, speakerSampleRate, nextStreamer) +// The beep.Seq already playing will seamlessly transition to the next stream. +``` + +**Crossfade architecture:** +```go +// Use a Mixer as the root streamer instead of a single chain. +type CrossfadeMixer struct { + mixer beep.Mixer + fadeInMs int + fadeOutMs int +} + +// When transitioning: +// 1. Create fade-out volume wrapper on current stream +// 2. Create fade-in volume wrapper on next stream +// 3. Add both to mixer +// 4. Use beep.StreamerFunc to drive the volume ramps over time +``` + +**Key integration changes:** +- The `Player` struct currently uses `speaker.Play(beep.Seq(...))` for single-stream playback. For gapless/crossfade, switch to a persistent `beep.Mixer` registered with the speaker once at init. +- Add/remove streams from the mixer rather than calling `speaker.Play()` per track. +- The `beep.Callback` for end-of-track still works but fires per-stream in the mixer, not per-speaker-play. +- Pre-decoding the next track requires knowing what the next track IS. This means the player needs awareness of the queue (currently it only knows about the current file). Wire this via a "next track provider" interface. + +### 6. Plugin System Architecture + +| Technology | Version | Purpose | Why | +|------------|---------|---------|-----| +| **Go `plugin` package** | stdlib | ❌ NOT recommended | Linux-only, same Go version required, fragile. | +| **hashicorp/go-plugin** | — | ❌ NOT recommended | gRPC-based, heavy for a desktop app, designed for server-side tools. | +| **Custom interface + registration** | — | ✅ Recommended | Define Go interfaces for backend hooks. Plugins implement interfaces and register at init. | + +**Confidence:** MEDIUM — plugin architecture is inherently design-specific. No off-the-shelf solution fits perfectly. + +**Recommended approach: Compiled-in plugin system with runtime-loaded UI** + +**Backend plugins (Go):** +```go +// backend/plugin/api.go +package plugin + +// Plugin is the interface all backend plugins must implement. +type Plugin interface { + ID() string + Name() string + Version() string + Init(ctx PluginContext) error + Shutdown() error +} + +// PluginContext provides access to app services. +type PluginContext struct { + DB *database.DB + Events EventEmitter + Config ConfigAccess + Logger *slog.Logger +} + +// Hook interfaces — plugins implement the ones they care about. +type OnTrackChangeHook interface { + OnTrackChange(track TrackInfo) error +} +type OnScanCompleteHook interface { + OnScanComplete(metrics ScanMetrics) error +} +``` + +For v1.1, backend plugins are compiled into the binary (via Go build tags or registration in main.go). True dynamic loading can come later via process-based plugins (subprocess + JSON-RPC). + +**Frontend plugins (TypeScript/Lit):** +- Plugins provide Lit web components that register themselves via `customElements.define()`. +- The layout system (see below) allows placing plugin components in UI sections. +- Plugin JS bundles are loaded at runtime from a plugins directory via dynamic `import()`. + +**No new Go dependencies needed** for the initial plugin system. The complexity is in API design, not in libraries. + +### 7. Layout Customization System + +| Technology | Purpose | Why | +|------------|---------|-----| +| **Existing: Lit + config.toml** | Section-based layout config | The existing TOML config system and Lit component architecture are sufficient. No new dependencies needed. | + +**Confidence:** HIGH — this is a UI architecture problem, not a library problem. + +**Architecture:** +```toml +# config.toml additions: +[Layout] + [Layout.Sidebar] + components = ["navigation", "now-playing-art"] + width = 250 + + [Layout.MainPanel] + components = ["track-list"] + + [Layout.BottomBar] + components = ["audio-player", "queue-mini"] +``` + +**Frontend implementation:** +```typescript +// A layout-section component that renders configured child components +@customElement('layout-section') +class LayoutSection extends LitElement { + @property() section: string = ''; + @property({ type: Array }) components: string[] = []; - p.mu.Lock() - p.restoreStateLocked() - p.mu.Unlock() -} - -// AFTER (single acquisition): -func (p *Player) SetContext(ctx context.Context) { - p.mu.Lock() - defer p.mu.Unlock() - p.ctx = ctx - p.restoreStateLocked() + render() { + return html`${this.components.map(name => { + const tag = document.createElement(name); + return tag; + })}`; + } } ``` -**Why:** Between the two lock acquisitions, another goroutine can modify state. The combined lock makes the set-context-and-restore atomic. +**Component registry pattern:** +```typescript +// Each component declares its constraints +interface LayoutComponent { + tagName: string; + displayName: string; + minWidth?: number; + minHeight?: number; + allowedSections: string[]; +} -### Pattern: Lock Ordering Documentation +const registry = new Map(); +``` -The player already documents its lock ordering rule: "acquire `p.mu` BEFORE `speaker.Lock()`." This is correct and critical. The `go p.onPlaybackFinished()` dispatch from the beep callback is essential — removing the goroutine dispatch would deadlock because the beep callback holds `speaker.Lock()` and `onPlaybackFinished` acquires `p.mu`. +**No new npm dependencies needed.** Lit's `customElements.define()` provides the dynamic component loading mechanism. The config system already handles TOML persistence and live reload. -**Recommendation:** Add a `// Lock ordering:` comment block to the Queue and Library structs as well, even though they only have one lock each. Document what operations must NOT hold the lock (event emission, player callbacks). +### 8. Smart Playlists +| Technology | Purpose | Why | +|------------|---------|-----| +| **Existing: SQLite + sqlc** | Dynamic query builder for filter rules | Smart playlists are SQL WHERE clauses stored as structured data. No new dependencies needed. | + +**Confidence:** HIGH — smart playlists are a database query problem. + +**Architecture:** ```go -// Queue manages an ordered list of tracks for playback. -// -// Concurrency: q.mu protects all mutable fields. Event emission -// (emitQueueChanged, etc.) is called WITH q.mu held because the -// Wails EventsEmit is non-blocking. The playbackFinishedHandler -// (auto-advance) re-enters the queue via AddTrack/Next, so it -// must NOT be called while holding q.mu. -type Queue struct { - mu sync.Mutex +// Smart playlist rule stored in SQLite +type SmartPlaylistRule struct { + Field string // "genre", "year", "artist", "play_count", "date_added" + Operator string // "equals", "contains", "greater_than", "less_than", "between" + Value string // The comparison value(s) +} + +type SmartPlaylist struct { + ID int64 + Name string + Rules []SmartPlaylistRule // Stored as JSON in SQLite + MatchAll bool // AND vs OR for combining rules + SortBy string + SortOrder string + Limit int // 0 = unlimited +} +``` + +**Query generation (not sqlc — dynamic WHERE clauses):** +```go +// Hand-crafted SQL builder for smart playlists. +// Cannot use sqlc because the WHERE clause is dynamic. +func (sp *SmartPlaylist) BuildQuery() (string, []any) { + // Build parameterized query from rules. + // Always use parameterized queries — never interpolate values. +} +``` + +**Schema addition:** New `smart_playlists` table with JSON rules column. New migration in the existing `PRAGMA user_version` system. + +**No new dependencies needed.** The existing `encoding/json` handles rule serialization. + +### 9. Customizable Keyboard Shortcuts + +| Technology | Purpose | Why | +|------------|---------|-----| +| **Existing: Wails runtime + config.toml + Lit** | Frontend keyboard event handling with configurable bindings | Keyboard shortcuts are a frontend concern in WebView. No new dependencies. | + +**Confidence:** HIGH — standard web keyboard event handling. + +**Architecture:** +```toml +# config.toml additions: +[KeyboardShortcuts] +play_pause = "Space" +next_track = "MediaTrackNext" +prev_track = "MediaTrackPrevious" +volume_up = "ArrowUp" +volume_down = "ArrowDown" +seek_forward = "ArrowRight" +seek_backward = "ArrowLeft" +toggle_queue = "Q" +search = "Ctrl+F" +``` + +**Frontend implementation:** +```typescript +// Global keyboard handler — listens on document, maps keys to actions +class KeyboardShortcutManager { + private bindings: Map; // key combo → action name + private actions: Map void>; // action name → handler + + handleKeyDown(e: KeyboardEvent) { + const combo = this.normalizeCombo(e); + const action = this.bindings.get(combo); + if (action) { + e.preventDefault(); + this.actions.get(action)?.(); + } + } +} +``` + +**No new dependencies needed.** The Web platform's `KeyboardEvent` API provides everything. Store bindings in TOML config, load on startup, emit config change events on update. + +### 10. Scan Cancellation + +| Technology | Purpose | Why | +|------------|---------|-----| +| **Existing: `context.WithCancel`** | Cancel in-progress library scan | Go's context cancellation is the standard pattern. The scan pipeline already uses `errgroup` which respects context cancellation. | + +**Confidence:** HIGH — standard Go pattern. + +**Implementation:** +```go +// In Library struct: +type Library struct { + scanCancel context.CancelFunc // nil when no scan is running // ... } + +func (l *Library) Scan() { + ctx, cancel := context.WithCancel(l.ctx) + l.scanCancel = cancel + defer func() { l.scanCancel = nil }() + + // Pass ctx to errgroup and all scan phases + g, gctx := errgroup.WithContext(ctx) + // Workers check gctx.Done() and exit early +} + +func (l *Library) CancelScan() { + if l.scanCancel != nil { + l.scanCancel() + } +} ``` -### Testing Pattern: Race Detector as Test Oracle +**No new dependencies needed.** The existing `golang.org/x/sync/errgroup` already propagates context cancellation to worker goroutines. + +--- + +## Alternatives Considered + +| Category | Recommended | Alternative | Why Not | +|----------|-------------|-------------|---------| +| MP3 tag writing | n10v/id3v2 v2 | bogem/id3v2 (old path) | Same library — `n10v/id3v2` is the current canonical path after maintainer rename | +| FLAC tag writing | go-flac/go-flac + flacvorbis | mewkiz/flac | mewkiz/flac is a decoder/encoder, not a metadata editor. Would require full re-encode to change tags. | +| MusicBrainz client | Custom HTTP client | michiwend/gomusicbrainz | Unmaintained, XML-only, missing browse API, no Go modules initially | +| MusicBrainz client | Custom HTTP client | go-musicbrainzws2 (SourceHut) | Low adoption, hard to verify maintenance, adds unfamiliar dependency | +| Gapless/crossfade | beep.Mixer (existing) | External audio library | beep already provides all needed primitives (Mixer, Seq, Volume, Resample) | +| Plugin system | Interface-based registration | hashicorp/go-plugin | gRPC overhead is inappropriate for a desktop app; designed for distributed systems | +| Plugin system | Interface-based registration | Go `plugin` package | Linux-only, same Go version required, CGo required for loading, extremely fragile | +| Plugin system | Interface-based registration | Wasm runtime (wazero) | Massive complexity for v1.1; good future option for sandboxed plugins | +| Smart playlists | Dynamic SQL builder | SQLite views | Views can't be parameterized at query time; rules need runtime evaluation | +| Keyboard shortcuts | Web KeyboardEvent API | Frontend hotkey library | No library needed for the scope of shortcuts in a music player | + +--- + +## What NOT to Add + +These are things the existing stack already handles. Do NOT add duplicate libraries: + +| Capability | Already Handled By | DON'T Add | +|-----------|-------------------|-----------| +| Tag reading | `github.com/dhowden/tag` | Any other tag reading library — keep dhowden/tag for the scan pipeline | +| Audio decoding | `gopxl/beep/v2` (mp3, flac, vorbis, wav) | Any other audio decoder | +| Config persistence | `BurntSushi/toml` | YAML, JSON, or any other config library | +| Database | `modernc.org/sqlite` | Any other database or ORM | +| HTTP client | Go stdlib `net/http` | Any HTTP client library for MusicBrainz | +| JSON parsing | Go stdlib `encoding/json` | Any JSON library for MusicBrainz responses | +| Concurrency | Go stdlib `context`, `sync`, `golang.org/x/sync` | Any additional concurrency primitives | +| Frontend reactivity | Lit 3.2.1 + @lit-labs/signals | Any state management library | +| Virtual scrolling | @lit-labs/virtualizer | Any other virtual scrolling solution | + +--- + +## Installation ```bash -# Already in Makefile — verify this is the exact command: -make test # → go test -tags webkit2_41 -race -count=1 -timeout 120s ./... +# New Go dependencies (tag writing + FLAC metadata): +go get github.com/n10v/id3v2/v2@v2.1.4 +go get github.com/go-flac/go-flac/v2 +go get github.com/go-flac/flacvorbis/v2 + +# No new frontend (npm) dependencies needed for v1.1. +# All features use existing Lit + Web platform APIs. ``` -The race detector is the most valuable tool here. Every new test implicitly checks for races when run with `-race`. No additional tooling needed — just write tests that exercise concurrent paths: +**Total new dependencies: 3 Go packages, 0 npm packages.** -```go -func TestQueueSetContextRace(t *testing.T) { - q := NewQueue(slog.Default(), testDB) - - // Simulate Wails calling SetContext while queue operations run. - var wg sync.WaitGroup - wg.Add(2) - go func() { - defer wg.Done() - q.SetContext(context.Background()) - }() - go func() { - defer wg.Done() - q.GetState() // reads under lock - }() - wg.Wait() -} -``` - -### What NOT to Do - -| Anti-Pattern | Why It's Wrong | Instead | -|---|---|---| -| `sync.RWMutex` for Queue/Player | These structs have frequent writes AND reads from multiple goroutines on the same timeline. RWMutex only helps when reads vastly outnumber writes and are long-running. Desktop event-driven access patterns don't benefit. | Keep `sync.Mutex`. Simpler, fewer bugs. | -| Channel-based state management | Replacing mutexes with channels for Queue state would require rewriting all methods. The current mutex pattern is correct, just under-applied. | Fix the races by adding lock acquisitions to SetContext methods. | -| `sync.Map` for entityCache | `sync.Map` is optimized for concurrent reads from many goroutines. The entityCache is accessed from a single DB-writer goroutine. It would add overhead with zero benefit. | Keep plain maps (already correct). | -| Package-level mutex for startupErr | A package-level mutex is worse than the disease. | Move `startupErr` to a field on `YellowJacketApp` struct. | +This is intentionally minimal. The v1.1 features are primarily architecture and design challenges, not library selection challenges. The existing stack is comprehensive enough that most features require new code, not new dependencies. --- -## 2. SQLite WAL Mode Optimization — Priority: HIGH +## Integration Points with Existing Stack -**Confidence:** HIGH — based on SQLite official docs (sqlite.org/wal.html), modernc.org/sqlite driver docs, and codebase analysis. +### Tag Editing → Player Coordination +The player holds an open file handle (`p.currentFile`) during playback. Tag writing libraries also need exclusive file access. The workflow must be: +1. Player.Pause() or Player.Stop() — release the file +2. Write tags via id3v2/go-flac +3. Rescan the file's metadata into the database +4. Player.LoadFile() with the same path — resume -### Current Setup Analysis +### MusicBrainz → Database Caching +MusicBrainz API responses should be cached in SQLite (new tables: `mb_cache_artists`, `mb_cache_releases`, etc.) with a TTL column. This reuses the existing database infrastructure and avoids redundant API calls. The 1-request-per-second rate limit makes caching essential for a responsive UI. -The database initialization is solid: -- WAL mode via `?_journal_mode=WAL` in DSN (**correct**) -- `_busy_timeout=5000` — 5 second busy wait (**correct**, prevents SQLITE_BUSY in most cases) -- `SetMaxOpenConns(1)` — single writer (**correct**, required for pure-Go driver) -- `PRAGMA foreign_keys = ON` (**correct**) +### Gapless/Crossfade → Speaker Architecture +Current: `speaker.Play()` called per track, creates new beep.Seq each time. +New: Register a persistent `beep.Mixer` with the speaker once at init. Add/remove per-track streamers to the mixer. This is the biggest architectural change — it affects Player, Queue auto-advance, and the playback-finished callback chain. -### Missing PRAGMAs to Add +### Smart Playlists → Existing Query Infrastructure +Smart playlists generate SQL queries against the existing `track_metadata` VIEW and related tables. They use the same `*database.DB` connection with the same `SetMaxOpenConns(1)` constraint. Rules are stored as JSON in a new `smart_playlists` table (schema migration via existing `PRAGMA user_version` system). -```go -// Add after foreign_keys pragma in NewDB(): -pragmas := []string{ - "PRAGMA foreign_keys = ON", - "PRAGMA synchronous = NORMAL", // WAL-safe, much faster - "PRAGMA cache_size = -8000", // 8MB page cache (default is -2000 = 2MB) - "PRAGMA mmap_size = 67108864", // 64MB memory-mapped I/O - "PRAGMA temp_store = MEMORY", // Temp tables in memory - "PRAGMA optimize", // Run at connection open -} -``` +### Layout Customization → Config + Frontend +New `[Layout]` section in config.toml, loaded via existing `BurntSushi/toml` config system. Layout changes emit config change events via existing Wails event bus. Frontend components register themselves in a component registry and the layout section components render them dynamically. -**Why `synchronous = NORMAL`:** In WAL mode, NORMAL provides durability against process crashes (only power loss can cause data loss of the last transaction). FULL is the default and fsyncs the WAL on every commit, which is unnecessary for a desktop music player where the data can be rescanned from disk. - -**Why `cache_size = -8000`:** The negative value means 8000 KiB (8MB). The default 2MB is fine for small databases but YellowJacket libraries can have 50k+ tracks. Larger cache reduces disk I/O for repeated queries (all-tracks, search, queue operations). - -**Why `mmap_size`:** Memory-mapped I/O lets SQLite read pages directly from the OS page cache. 64MB covers most music library databases entirely. With modernc.org/sqlite (pure Go), mmap is handled by the underlying C translation and works on Linux/macOS/Windows. - -**Why `PRAGMA optimize` at open:** Runs `ANALYZE` on tables where the optimizer thinks statistics are stale. Zero cost if stats are fresh. - -### Add `PRAGMA optimize` at Shutdown - -```go -// In app.go OnShutdown: -func (a *YellowJacketApp) OnShutdown(ctx context.Context) { - // ... existing cleanup ... - _, _ = a.db.ExecContext("PRAGMA optimize") // Update query planner stats -} -``` - -SQLite docs recommend running `PRAGMA optimize` at close to ensure statistics are written for the next session. - -### Query Consolidation: FTS5 JOIN Deduplication - -The codebase has 5 copies of the same FTS5 JOIN pattern. Extract it: - -```go -// backend/database/search.go - -// ftsMetadataJoin is the common JOIN clause for resolving audio file -// metadata through the recording → artist_credit → release_group chain. -// Use with "FROM search_index si" or "FROM audio_files af" as the base. -const ftsMetadataJoin = ` - JOIN audio_files af ON af.id = si.rowid - LEFT JOIN recordings r ON af.recording_id = r.id - LEFT JOIN artist_credit ac ON r.artist_credit_id = ac.id - LEFT JOIN ( - SELECT recording_id, - MIN(release_group_id) AS release_group_id - FROM release_group_recordings - GROUP BY recording_id - ) rgr ON r.id = rgr.recording_id - LEFT JOIN release_groups rg ON rgr.release_group_id = rg.id -` -``` - -Then each search function references the constant instead of duplicating the SQL. This ensures schema changes only need one update. - -**Alternative:** Move these to sqlc queries where possible. The `SearchFTS` and `SearchFTSByFilename` functions can't easily use sqlc because the FTS5 `MATCH` syntax isn't well-supported by sqlc's parser. Keep them as hand-crafted SQL with the shared constant. Document why with a comment. - -### Queue Persistence: Incremental Updates - -The current `persistTracks()` does `DELETE ALL + INSERT ALL` on every mutation. For a queue with 1000 tracks, every add/remove/move rewrites all 1000 rows. - -**Pattern: Differential persistence for single-track operations:** - -```go -// For AddTrack — single INSERT instead of full rewrite: -func (q *Queue) persistAddTrack(track Track) { - err := q.db.Queries.InsertQueueTrack(q.db.Ctx, sqlcgen.InsertQueueTrackParams{ - AudioFileID: track.AudioFileID, - Position: track.Position, - }) - if err != nil { - q.logger.Error("Failed to persist added track", "err", err) - } -} - -// For RemoveTrack — single DELETE: -func (q *Queue) persistRemoveTrack(position int64) { - err := q.db.Queries.DeleteQueueTrackByPosition(q.db.Ctx, position) - if err != nil { - q.logger.Error("Failed to persist removed track", "err", err) - } -} -``` - -**Keep full rewrite for:** `SetQueue`, `RestoreState`, shuffle reordering — cases where the entire queue changes at once. - -**Estimated impact:** Reduces O(n) per-mutation writes to O(1) for the common case (add/remove single track). For a 5000-track queue, this eliminates ~10,000 unnecessary row writes per track operation. - -### What NOT to Do - -| Anti-Pattern | Why It's Wrong | Instead | -|---|---|---| -| Connection pooling (`SetMaxOpenConns > 1`) | modernc.org/sqlite is a single-writer database. Multiple connections cause SQLITE_BUSY errors. The current `SetMaxOpenConns(1)` is correct. | Keep `SetMaxOpenConns(1)`. | -| `_txlock=immediate` on all transactions | Immediate locking blocks all readers during writes. The default deferred locking only acquires a write lock when needed. For a desktop app with infrequent writes, deferred is fine. | Use immediate locking ONLY for critical write transactions (queue persistence) where you want to fail fast on contention. | -| Switching to `mattn/go-sqlite3` (CGo) | Adds CGo dependency, complicates cross-compilation, and the project constraint explicitly prohibits it. modernc.org/sqlite v1.45+ performance is within 10-20% of CGo for most workloads. | Stay on modernc.org/sqlite. | -| WAL2 mode | WAL2 is experimental in SQLite. Not available through any Go driver. | Stay on standard WAL. | - ---- - -## 3. Lit Web Component Performance — Priority: MEDIUM - -**Confidence:** MEDIUM — based on Lit official docs and @lit-labs/virtualizer usage in the codebase. - -### Current State - -The codebase already uses `@lit-labs/virtualizer` v2.1.1 in all list views (track-list, cover-grid, artists-view, genres-view, queue-panel). The virtualizer handles DOM recycling for large datasets. The main performance concerns are: - -1. **Eager full-library fetch on startup** — `libraryStore.eagerFetch()` loads all tracks, albums, artists, genres simultaneously -2. **Large component files** — 1400-2600 lines mixing concerns (though this is a code quality issue, not a performance issue per se) -3. **Rendering cost of metadata-heavy rows** — each track row has 16+ fields - -### Pattern: Lazy Loading Per View - -Replace `eagerFetch()` with on-demand loading: - -```typescript -class LibraryStore { - // Instead of fetching all four collections at construction: - constructor() { - EventsOn(Events.LibraryScanComplete, () => { - this.invalidate(); - }); - this.loadCoverSize(); - // Remove: this.eagerFetch(); - } - - // The existing getTracks/getAlbums already support lazy loading — - // they check for null and fetch if needed. The only change needed - // is removing eagerFetch() from the constructor. -} -``` - -**Why:** The existing `getTracks()`, `getAlbums()`, etc. already have null-check-and-fetch logic. The `eagerFetch()` in the constructor defeats this by loading everything upfront. Removing it means only the active view's data is fetched when first navigated to. - -**Risk:** First navigation to each view will have a brief loading delay. Mitigate with loading indicators (the `tracksLoading`/`albumsLoading` flags already exist). - -### Pattern: Minimize Re-renders with `guard` Directive - -For expensive computed values in templates (like filtered/sorted track lists), use Lit's `guard` directive to avoid recomputation: - -```typescript -import { guard } from 'lit/directives/guard.js'; - -// In render(): -${guard([this.tracks, this.sortColumn, this.sortDirection], () => - this.sortedTracks() -)} -``` - -**When to use:** For any computed property that depends on reactive properties but is expensive to compute (sorting 50k tracks, filtering, etc.). - -### Pattern: keyed Rendering for Virtualizer Lists - -Ensure virtualizer items have stable keys so DOM nodes are reused correctly when the list changes: - -```typescript -// The virtualizer uses index-based identity by default. -// For track lists that can be reordered (queue, playlists), -// provide a keyFunction: - track.filePath} - .renderItem=${(track: Track) => html`...`} -> -``` - -**Why:** Without stable keys, reordering a list causes the virtualizer to re-render every visible row. With keys, it reuses existing DOM nodes for rows that moved position. - -### What NOT to Do - -| Anti-Pattern | Why It's Wrong | Instead | -|---|---|---| -| Moving to React/Preact | The project uses Lit Web Components with Wails' WebView. Switching frameworks is explicitly out of scope and would require rewriting all 20+ components. | Stay on Lit 3.x. | -| Pre-rendering / SSR | Desktop app. No server. No need. | N/A | -| Replacing `@lit-labs/virtualizer` with a custom solution | The virtualizer is battle-tested and integrates with Lit's update lifecycle. A custom solution would need to handle the same edge cases (resize, scroll restoration, dynamic heights). | Keep `@lit-labs/virtualizer`. File bugs if issues are found. | -| `requestAnimationFrame` batching for store updates | Lit already batches updates at microtask timing. Adding rAF batching would add latency without benefit. | Let Lit handle batching. | - ---- - -## 4. Go Testing Strategies — Priority: HIGH - -**Confidence:** HIGH — based on Go standard library patterns and codebase-specific analysis. - -### Strategy: In-Memory SQLite for Database Tests - -modernc.org/sqlite supports in-memory databases. Use them for fast, isolated tests: - -```go -// backend/database/testhelper_test.go (shared across test files in the package) - -func newTestDB(t *testing.T) *database.DB { - t.Helper() - // Use ":memory:" with shared cache so the connection sees the same DB. - // The query string params mirror production config. - db, err := database.NewTestDB(":memory:?_journal_mode=WAL&_busy_timeout=5000") - if err != nil { - t.Fatal(err) - } - t.Cleanup(func() { db.Close() }) - return db -} -``` - -**For this to work, add a `NewTestDB` constructor to the database package** that accepts a custom DSN instead of computing one from the user data directory: - -```go -// backend/database/database.go - -// NewTestDB creates a database connection with a caller-provided DSN. -// Intended for unit tests that use in-memory databases. -func NewTestDB(dsn string) (*DB, error) { - // Same initialization logic as NewDB but with custom DSN. - // Runs migrations, sets pragmas, etc. -} -``` - -**Why in-memory:** Tests run in ~1ms instead of ~50ms. No filesystem cleanup. No conflict between parallel tests. Each test gets a fresh database. - -**Important:** SQLite in-memory databases with `SetMaxOpenConns(1)` work correctly — the single connection sees a consistent view. No need for shared cache mode with a single connection. - -### Strategy: Extract Pure Functions from Player - -The player has testable logic that doesn't need audio hardware: - -```go -// Volume math — currently inline in player methods: -func userVolumeToBeep(userVolume int) (volume float64, silent bool) { - if userVolume <= 0 { - return 0, true - } - // Convert 0-100 linear user volume to beep's logarithmic Volume field. - // Base is 2, so Volume = log2(userVolume/MaxUserVol * range) - // This is the math currently embedded in Set/GetVolume methods. - return math.Log2(float64(userVolume) / float64(MaxUserVol)), false -} - -// State serialization — currently inline in persist/restore: -func serializePlayerState(state State, volume int, filePath string) PlayerStateRow { ... } -func deserializePlayerState(row PlayerStateRow) (State, int, string) { ... } -``` - -**Why:** These pure functions can be tested exhaustively (edge cases: volume 0, volume 100, max uint64 trackChangeID, empty filepath) without any speaker initialization or Wails context. - -### Strategy: Interface-Based Mocking for Queue Tests - -The `Queue` depends on `TrackLoader` (player) and `*database.DB`. The `TrackLoader` is already an interface — perfect for testing: - -```go -// backend/queue/queue_test.go - -type mockPlayer struct { - loaded []string - playing bool - position int -} - -func (m *mockPlayer) LoadFile(path string) error { - m.loaded = append(m.loaded, path) - return nil -} -func (m *mockPlayer) Play() error { m.playing = true; return nil } -func (m *mockPlayer) IsPlaying() bool { return m.playing } -func (m *mockPlayer) CurrentPositionSeconds() (int, error) { return m.position, nil } -func (m *mockPlayer) UnloadTrack() { m.playing = false } - -func TestSetQueuePlaysFirstTrack(t *testing.T) { - db := newTestDB(t) - // Seed test tracks into db... - - q := queue.NewQueue(slog.Default(), db) - player := &mockPlayer{} - q.SetPlayer(player) - q.SetContext(context.Background()) - - q.SetQueue([]string{"/music/a.mp3", "/music/b.mp3"}, 0, false) - - if len(player.loaded) == 0 { - t.Fatal("expected player to load a file") - } - if player.loaded[0] != "/music/a.mp3" { - t.Errorf("expected first track, got %s", player.loaded[0]) - } -} -``` - -### Strategy: Config Round-Trip Testing - -```go -func TestConfigRoundTrip(t *testing.T) { - dir := t.TempDir() - path := filepath.Join(dir, "config.toml") - - original := config.DefaultConfig() - original.Theme.AccentColor = "#ff0000" - - err := config.Save(path, original) - if err != nil { - t.Fatal(err) - } - - loaded, err := config.Load(path) - if err != nil { - t.Fatal(err) - } - - if loaded.Theme.AccentColor != "#ff0000" { - t.Errorf("accent color not preserved: got %s", loaded.Theme.AccentColor) - } -} -``` - -### Strategy: Event Name Parity Validation - -Build-time check that Go and TypeScript event names match: - -```go -// backend/events/events_test.go - -func TestEventNameParity(t *testing.T) { - // Read the Go events constants via reflection or by parsing the source. - // Read frontend/src/events.ts. - // Compare the sets. - - goEvents := extractGoEventNames(t) // parse events.go - tsEvents := extractTSEventNames(t) // parse events.ts - - for name := range goEvents { - if _, ok := tsEvents[name]; !ok { - t.Errorf("Go event %q not found in TypeScript events.ts", name) - } - } - for name := range tsEvents { - if _, ok := goEvents[name]; !ok { - t.Errorf("TypeScript event %q not found in Go events.go", name) - } - } -} -``` - -**Implementation note:** Parse events.go for `const ( ... )` block string values. Parse events.ts for the `Events` object literal values. This is a ~50-line test that prevents silent event name drift forever. - -### Test Priority Order - -| Package | Why First | Test Count Estimate | -|---|---|---| -| `queue` | Central to playback, most concurrency issues, persistence bugs | ~15-20 tests | -| `database` | FTS5 edge cases, migration correctness, search behavior | ~10-15 tests | -| `config` | Round-trip fidelity, defaults, validation, permissions | ~8-10 tests | -| `player` (pure logic only) | Volume math, state serialization | ~5-8 tests | -| `events` | Parity check | 1 test | -| `library` | Scan logic is complex but depends on filesystem fixtures | ~10 tests (lower priority) | - -### What NOT to Do - -| Anti-Pattern | Why It's Wrong | Instead | -|---|---|---| -| Test doubles for SQLite (full mock DB layer) | In-memory SQLite IS the test double. It runs the same SQL engine with the same behavior. Mocking at the `*sql.DB` level loses all SQL correctness checking. | Use `:memory:` SQLite databases. | -| `testify` or other assertion libraries | The project uses standard `testing` only. Adding assertion libraries creates style inconsistency and dependency bloat. | Use `t.Errorf`, `t.Fatal`, and `if` checks. | -| Integration tests in CI for player | The player requires an audio output device. CI runners don't have one. The existing skip mechanism (`YELLOWJACKET_INTEGRATION`) is correct. | Extract pure functions from player; leave hardware tests as opt-in integration tests. | -| Coverage targets | The PROJECT.md explicitly says "Tests support refactoring, not standalone goal." Coverage targets incentivize low-value tests. | Test critical paths: queue operations, search, config round-trip, event parity. | - ---- - -## 5. beep/v2 Audio Library Patterns — Priority: MEDIUM - -**Confidence:** MEDIUM — based on beep wiki docs, gopxl/beep v2 API, and codebase lock ordering analysis. - -### Lock Ordering: The One Rule - -beep/v2 has a global speaker lock (`speaker.Lock()/speaker.Unlock()`). The player has its own `sync.Mutex`. The existing documented rule is correct: - -> **Always acquire `p.mu` BEFORE `speaker.Lock()`.** - -The critical implementation detail: the beep callback (end-of-track) runs with `speaker.Lock()` held. The player dispatches to a goroutine (`go p.onPlaybackFinished()`) so that it can safely acquire `p.mu`. **This goroutine dispatch MUST NOT be removed.** Removing it causes deadlock: - -``` -Deadlock scenario without goroutine dispatch: -1. beep callback fires (speaker lock HELD) -2. onPlaybackFinished tries to acquire p.mu → blocks if another goroutine holds p.mu -3. That other goroutine calls speaker.Lock() → blocks because speaker lock is held by beep -4. DEADLOCK -``` - -### Pattern: Speaker Lock Scope Minimization - -The current code correctly locks the speaker only when mutating streamer state: - -```go -func (p *Player) startPaused() { - speaker.Lock() - p.control.Paused = true - speaker.Unlock() - // speaker.Play registers streamers — does its own locking. - speaker.Play(beep.Seq(p.speakerStreamer, beep.Callback(func() { - go p.onPlaybackFinished() - }))) - p.state = Paused -} -``` - -**Keep speaker.Lock() regions as small as possible.** Never do I/O, logging, or event emission while holding the speaker lock. - -### Pattern: Streamer Chain Lifecycle - -The current `updateStreamers()` method correctly rebuilds the entire chain (base → resample → ctrl → volume) on each track load. This is the right pattern for beep — streamer chains are cheap to construct and shouldn't be reused across tracks. - -**One improvement:** The `updateStreamers` method preserves volume state across track changes, which is correct. But it could also preserve the paused state: - -```go -func (p *Player) updateStreamers(newBaseStreamer beep.StreamSeeker, sr beep.SampleRate) error { - // ...existing code... - - // Preserve existing pause state across track changes. - prevPaused := false - if p.control != nil { - prevPaused = p.control.Paused - } - - p.control = &beep.Ctrl{Streamer: p.resampled, Paused: prevPaused} - // ... -} -``` - -### Extractable Pure Logic from Player - -These functions can be extracted and tested without audio hardware: - -| Function | Current Location | Pure? | Test Value | -|---|---|---|---| -| Volume conversion (user 0-100 ↔ beep logarithmic) | Inline in `SetVolume`/`GetVolume` | Yes | Edge cases: 0, 1, 50, 100 | -| Display position calculation | `displayPositionSecsLocked()` | Yes (math only) | Seek position rounding, track length boundary | -| Track info construction | `getCurrentTrackInfoLocked()` | Mostly (reads state) | Null file, missing metadata | -| Resample quality mapping | Currently hardcoded `4` | Yes (when made configurable) | Quality 1-6 range validation | - -### What NOT to Do - -| Anti-Pattern | Why It's Wrong | Instead | -|---|---|---| -| Replacing beep with a lower-level audio library (oto, portaudio) | beep provides the streamer composition model (Seq, Ctrl, Volume, Resample) that the player relies on. Dropping to oto means reimplementing all of this. | Stay on beep/v2. File issues for bugs. | -| Multiple speaker.Init calls | `speaker.Init` can only be called once (or after `speaker.Close()`). Calling it again is undefined behavior. The current "init once on startup" is correct. | Keep single Init on startup. If sample rate needs to change, the entire speaker must be closed and reinitialized. | -| Holding p.mu during speaker.Play() | `speaker.Play()` does its own internal locking. Holding p.mu during the call is safe but unnecessary — and if beep ever calls back synchronously (which it currently doesn't for `Play()`), could cause issues. | Release p.mu before speaker.Play() if possible, or document why it's held. | - ---- - -## Development Tools: Existing Stack Assessment - -### Already Correct — No Changes Needed - -| Tool | Version | Assessment | -|---|---|---| -| golangci-lint v2 | v2.9.0 | Strict config already in place. Catches most issues. | -| Race detector | Go 1.25 | Already enabled in `make test`. | -| lefthook | v1.13.6 | Pre-commit hooks run vet, lint, codegen-check, typecheck. | -| govulncheck | v1.1.4 | Vulnerability scanning for Go dependencies. | -| sqlc | v1.30.0 | SQL-to-Go code generation for type-safe queries. | -| pprof profiling | Built-in | Dev-only pprof server on localhost:6060, block/mutex profiling enabled. | -| Vite + HMR | v7.0.0 | Fast frontend rebuilds during development. | - -### Recommended Addition: `t.TempDir()` for Test Isolation - -Go 1.15+ provides `t.TempDir()` which auto-cleans. Use for config tests and any test that needs filesystem: - -```go -func TestConfigSave(t *testing.T) { - dir := t.TempDir() // cleaned up automatically - path := filepath.Join(dir, "config.toml") - // ... -} -``` - -### Recommended Addition: `t.Parallel()` for Independent Tests - -Mark tests that don't share state as parallel to speed up the test suite: - -```go -func TestQueueAddTrack(t *testing.T) { - t.Parallel() // runs concurrently with other parallel tests - db := newTestDB(t) // each test gets its own in-memory DB - // ... -} -``` - -**Important:** Only use `t.Parallel()` when each test creates its own database and mock player. Tests that share state (global variables, singleton stores) cannot be parallel. - ---- - -## Version Compatibility - -| Package | Current Version | Compatible With | Notes | -|---|---|---|---| -| Go | 1.25.0 | All dependencies | Go 1.25 introduced `t.Context()`, tool directive in go.mod | -| modernc.org/sqlite | v1.45.0 | SQLite 3.51.x | Match modernc.org/libc version exactly per upstream warning | -| beep/v2 | v2.1.1 | ebitengine/oto v3.3.3 | oto is the audio backend; version locked through go.mod | -| Lit | ^3.2.1 | @lit-labs/virtualizer ^2.1.1 | Labs packages are experimental but stable for virtualizer | -| @lit-labs/signals | ^0.2.0 | Lit ^3.2.1 | Used for signal-based reactivity; experimental API may change | -| sqlc | v1.30.0 | modernc.org/sqlite | sqlc generates code for `database/sql` interface; driver-agnostic | +### Plugin System → Everything +Backend plugins get a `PluginContext` with access to DB, events, config, logger. Frontend plugins load as JS modules via `import()` and register Lit web components. Both hook into the existing architecture rather than requiring new infrastructure. --- ## Sources -- SQLite WAL documentation: https://www.sqlite.org/wal.html — **HIGH confidence** (official docs, updated 2025-05-31) -- SQLite PRAGMA documentation: https://www.sqlite.org/pragma.html — **HIGH confidence** (official docs) -- modernc.org/sqlite API: https://pkg.go.dev/modernc.org/sqlite@v1.46.1 — **HIGH confidence** (official Go package docs) -- gopxl/beep wiki — Composing and controlling: https://github.com/gopxl/beep/wiki/Composing-and-controlling — **HIGH confidence** (official beep docs) -- Lit rendering docs: https://lit.dev/docs/components/rendering/ — **HIGH confidence** (official Lit docs) -- Go race detector: https://go.dev/doc/articles/race_detector — **HIGH confidence** (official Go docs) -- Codebase analysis: `.planning/codebase/CONCERNS.md`, `.planning/codebase/STACK.md` — **HIGH confidence** (direct code inspection) -- beep speaker.Lock() behavior: inferred from beep wiki and codebase lock ordering comments — **MEDIUM confidence** (documented in code but not in beep's API docs) +- n10v/id3v2: https://github.com/n10v/id3v2 — **HIGH confidence** (verified GitHub repo, 359 stars, 43 releases, MIT license) +- go-flac/go-flac: https://github.com/go-flac/go-flac — **MEDIUM confidence** (verified, 44 stars, v2 module available, Apache-2.0 license) +- go-flac/flacvorbis: https://github.com/go-flac/flacvorbis — **MEDIUM confidence** (verified, 11 stars, v2 module available, Apache-2.0 license) +- MusicBrainz API: https://musicbrainz.org/doc/MusicBrainz_API — **HIGH confidence** (official documentation, comprehensive) +- MusicBrainz rate limiting: https://musicbrainz.org/doc/MusicBrainz_API/Rate_Limiting — **HIGH confidence** (official) +- beep v2.1.1 API: https://pkg.go.dev/github.com/gopxl/beep/v2 — **HIGH confidence** (official Go package docs, verified Mixer, Seq, Volume types) +- beep Mixer documentation: verified from pkg.go.dev — Add(), Clear(), KeepAlive(), Stream() methods confirmed +- michiwend/gomusicbrainz: https://github.com/michiwend/gomusicbrainz — **HIGH confidence** (verified, 64 stars, only search+lookup, no modules, effectively unmaintained) +- Go plugin package limitations: https://pkg.go.dev/plugin — **HIGH confidence** (official docs, Linux+macOS only, same Go version requirement documented) --- -*Stack research for: YellowJacket consolidation milestone* -*Researched: 2026-02-27* +*Stack research for: YellowJacket v1.1 Features & Extensibility* +*Researched: 2026-03-06* diff --git a/.planning/research/SUMMARY.md b/.planning/research/SUMMARY.md index cf2ea97..ee1b398 100644 --- a/.planning/research/SUMMARY.md +++ b/.planning/research/SUMMARY.md @@ -1,189 +1,204 @@ # Project Research Summary -**Project:** YellowJacket — Desktop Music Player Consolidation -**Domain:** Go/Wails/Lit desktop application — codebase quality & reliability improvement -**Researched:** 2026-02-27 +**Project:** YellowJacket v1.1 — Features & Extensibility +**Domain:** Desktop music player — feature expansion of existing Go/Wails/Lit/SQLite application +**Researched:** 2026-03-06 **Confidence:** HIGH ## Executive Summary -YellowJacket is a Go/Wails/Lit desktop music player with a functional feature set but known correctness issues: three data races in `SetContext` patterns, swallowed errors throughout the backend, zero test coverage on critical paths (queue, library, database, config), and O(n) queue persistence for single-track mutations. The consolidation milestone is not about new features — it's about making the existing codebase reliable, testable, and performant. The existing stack (Go 1.25, modernc.org/sqlite, beep/v2, Lit 3, sqlc) is correct and should not change. The work is purely internal quality improvement. +YellowJacket v1.1 adds 8 features to a well-structured existing codebase: tag editing, scan cancellation, smart playlists, customizable keyboard shortcuts, gapless playback + crossfade, MusicBrainz browser, layout customization, and a plugin system foundation. The research confirms this is overwhelmingly an **architecture and design challenge, not a library selection challenge**. Only 3 new Go packages are needed (tag writing for MP3 and FLAC); the remaining features build entirely on the existing stack (beep v2.1.1, SQLite, Lit 3.2.1, Wails v2, stdlib). Zero new npm dependencies are required. -The recommended approach is **tests-first, then refactoring**. The research consistently shows that every optimization and consolidation change (FTS5 query deduplication, queue incremental persistence, lazy library loading) is risky without tests to verify behavior is preserved. The critical dependency chain is: fix concurrency bugs → build test infrastructure → write tests → refactor safely. This ordering emerges independently from all four research files — STACK recommends in-memory SQLite testing, FEATURES shows test infrastructure as the top enabler, ARCHITECTURE proposes the same phase ordering, and PITFALLS warns that every refactoring without tests creates invisible regressions. +The recommended approach is **integration-first**: every feature slots into established codebase patterns (two-phase init, event-driven sync, mutex-protected state, sqlc codegen, TOML config) rather than introducing new paradigms. Features vary dramatically in complexity — scan cancellation requires ~50 lines of changes to existing code, while gapless playback requires a fundamental restructuring of the audio pipeline. The build order should exploit this variance: ship quick wins first (scan cancel, keyboard shortcuts) to validate integration patterns, then tackle data model extensions (tag editing, smart playlists), then high-risk backend changes (gapless, MusicBrainz), and finally the extensibility foundations (layout, plugins). -The key risks are: (1) deadlock from player mutex + speaker lock ordering violations during refactoring, (2) FTS5 query consolidation silently changing search ranking, and (3) queue persistence migration losing queue state on restart. All three are mitigated by the same strategy: write characterization tests before changing the code. The player's lock ordering (`p.mu` before `speaker.Lock()`, goroutine dispatch in beep callback) is the one area requiring extreme caution — the recommendation is to extract pure testable logic and leave lock-sensitive paths alone unless absolutely necessary. +The primary risks are: (1) **tag writing corrupting audio files** — mitigated by write-to-temp-then-rename and blocking writes on playing files; (2) **gapless playback breaking the existing lock ordering and callback contract** — mitigated by pre-decoding in a separate goroutine and using beep's Mixer/Seq primitives; (3) **scan cancellation causing silent data loss** via orphan cleanup on partial scan data — mitigated by skipping orphan cleanup on cancelled scans; and (4) **MusicBrainz rate limiting** — mitigated by a strict 1 req/s rate limiter, aggressive SQLite caching, and proper User-Agent header. The plugin system is the highest architectural risk but is scoped to "foundation only" for v1.1, which limits blast radius. ## Key Findings ### Recommended Stack -The existing stack is correct. No changes needed. See [STACK.md](./STACK.md) for full details. +The existing stack is comprehensive. v1.1 adds only 3 new Go dependencies and 0 npm dependencies. This is the right call — most features are solved by new code, not new libraries. -**Core technologies (all already in use):** -- **Go 1.25 + modernc.org/sqlite v1.45**: Pure-Go SQLite driver with WAL mode, `SetMaxOpenConns(1)` — correct setup, needs missing PRAGMAs (`synchronous=NORMAL`, `cache_size=-8000`, `mmap_size=67108864`) -- **beep/v2 + ebitengine/oto**: Audio playback with streamer composition — lock ordering documented, goroutine dispatch pattern critical -- **Lit 3 + @lit-labs/virtualizer**: Web components with virtual scrolling — already handles large lists, needs lazy loading instead of eager fetch -- **sqlc v1.30**: Type-safe SQL code generation — works well for standard queries, FTS5 queries must remain hand-crafted -- **golangci-lint v2, lefthook, govulncheck**: Already configured, no changes needed +**New dependencies (all Go):** +- **n10v/id3v2 v2.1.4**: MP3 tag writing (ID3v2.3/v2.4) — the only maintained pure-Go library with full write support (359 stars, active). Replaces nothing; `dhowden/tag` stays for reads. +- **go-flac/go-flac v2**: FLAC metadata block manipulation — low-level but the only Go option for FLAC tag writing. +- **go-flac/flacvorbis v2**: Vorbis comment read/write within FLAC files — companion to go-flac. -**Critical version note:** Match modernc.org/libc version exactly per upstream warning when updating modernc.org/sqlite. +**Reused from existing stack (no new deps):** +- **Gapless/Crossfade**: `beep.Mixer`, `beep.Seq`, `effects.Volume` — all already in beep v2.1.1. +- **MusicBrainz**: Custom HTTP client using stdlib `net/http` + `encoding/json`. Thin wrapper (~200 lines) beats unmaintained third-party clients. +- **Smart Playlists**: Dynamic SQL against existing `track_metadata` VIEW. No ORM needed. +- **Shortcuts**: Web platform `KeyboardEvent` API + TOML config persistence. +- **Layout**: Lit `customElements.define()` + component registry. CSS Container Queries for responsive components. +- **Plugins**: Interface-based Go hooks (compiled-in for v1.1) + dynamic JS module loading for frontend. + +**Critical version requirement:** OGG Vorbis and WAV tag writing should be deferred — no mature pure-Go libraries exist. MP3 + FLAC cover ~95% of music libraries. ### Expected Features -This is a consolidation milestone — "features" are quality improvements, not user-facing functionality. See [FEATURES.md](./FEATURES.md) for full details. +**Must have (table stakes):** +- Tag editing: single track + batch edit for title/artist/album/genre/year + write to file + DB sync +- Scan cancellation: cancel button, graceful stop (no DB corruption), progress reporting +- Smart playlists: filter by genre/year/artist, combine rules with AND, auto-update on library changes, save and name +- Keyboard shortcuts: play/pause, next/prev, volume, search focus, defaults that work out of box +- Gapless playback: no silence between tracks (this is expected by serious music listeners) +- Crossfade: on/off toggle with configurable duration (1-10 seconds) +- MusicBrainz browser: artist search, discography view, album track listing, rate limit compliance +- Layout: resizable panels, show/hide queue, persist across restarts -**Must fix (table stakes — codebase is unreliable without these):** -- Fix 3 SetContext data races (Queue, Library, Playlist) — textbook race, LOW effort -- Fix package-level `startupErr` → struct field — LOW effort -- Fix config file permissions (0o666 → 0o644) — one-line fix -- Fix swallowed errors in MPRIS callbacks and artist credit links — LOW effort -- Separate scan warnings from fatal errors in Library.Scan — MEDIUM effort -- Create in-memory SQLite test infrastructure (`database.NewTestDB()`) — MEDIUM effort, enables everything else -- Write unit tests for queue, library, database, config — HIGH effort, critical safety net +**Should have (differentiators):** +- Batch tag editing with preview/confirmation +- Smart playlists with random/limit results ("random 50 Jazz tracks") +- Per-album gapless (disable crossfade within albums) +- MusicBrainz response caching in SQLite +- Layout presets (Compact, Full, Mini player) +- Full shortcut customization UI with conflict detection +- Cover art assignment in tag editor -**Should do (significant quality improvement):** -- Consolidate duplicated FTS5 JOIN pattern (5+ copies → SQLite VIEW) — MEDIUM effort -- Optimize queue persistence to incremental updates — MEDIUM effort -- Remove `eagerFetch()` from library store constructor (lazy loading infrastructure already exists) — LOW effort -- Fix SetQueue Phase 2 redundant metadata lookups — LOW effort -- Add event name parity validation (Go ↔ TypeScript) — LOW effort -- Extract testable pure logic from Player (volume math, state serialization) — LOW effort - -**Defer (not this milestone):** -- Frontend component testing (expensive setup, backend is source of truth) -- Paginated data providers for 100k+ libraries (measure first) -- Full UI polish / transitions (CSS-only, independent) -- Rewriting the event system (works fine, just needs codegen parity check) +**Defer (v2+):** +- Tag-to-filename rename, undo/redo for tag edits +- Play count tracking and rating system (needed for advanced smart playlist rules) +- Plugin marketplace and dynamic Go plugin loading +- Auto-tag from MusicBrainz (this is Picard's domain) +- Detachable panels (Wails v2 limitation) +- OGG Vorbis tag writing +- DSP effects chain (equalizer, reverb) ### Architecture Approach -The architecture is sound and shouldn't change structurally. The consolidation work is about fixing correctness issues within the existing patterns and adding test infrastructure. See [ARCHITECTURE.md](./ARCHITECTURE.md) for full details. +Integration-first: 6 new backend packages + 5 new frontend stores/components slot into established patterns. Backend remains source of truth. Frontend stores are reactive mirrors. Events flow backend→frontend. Actions flow frontend→backend via Wails bindings. The one paradigm shift is the audio pipeline: switching from single-streamer to persistent `beep.Mixer` as the root speaker streamer. -**Six issues identified, in dependency order:** -1. **SetContext race fixes** — Add mutex guards to Queue, Library, Playlist `SetContext()`. Combine Player's double-lock into single acquisition. Move `startupErr` to struct field. -2. **Event name codegen** — Generate `frontend/src/events.ts` from `backend/events/events.go` using `go/ast`. Wire into `go generate` + pre-commit hook. -3. **Library store lazy loading** — Remove `eagerFetch()` from constructor. Lazy infrastructure already exists. Optional: paginated data providers for 100k+ libraries. -4. **Queue incremental persistence** — Use existing sqlc queries (`InsertQueueTrack`, `RemoveQueueTrackByPosition`, etc.) for single-track operations. Keep full rewrite for `SetQueue`/`Clear`. -5. **FTS5 query consolidation** — Create SQLite VIEW `track_metadata` encapsulating the 5-table JOIN. Migrate search queries to use VIEW. Keep inline JOINs in migrations. -6. **Test architecture** — `database.NewTestDB()` for in-memory SQLite. `internal/testdb` helper package. Mock only narrow interfaces (`TrackLoader`). Use `context.Background()` for Wails context in tests. +**Major new components:** +1. **`backend/tageditor/`** — Format-specific tag writing + DB cascade update + FTS5 re-index +2. **`backend/smartplaylist/`** — Rule-based dynamic query evaluation against `track_metadata` VIEW +3. **`backend/musicbrainz/`** — Rate-limited HTTP client + SQLite response cache +4. **`backend/shortcuts/`** — Shortcut registry mapping key combos to backend action handlers +5. **`backend/layout/`** — Section-based layout config read from TOML, exposed to frontend +6. **`backend/plugin/`** — Plugin manifest parsing, JS loader, hook registry, API surface + +**Modified components:** +- **`backend/player/`** — Gapless pre-loading, crossfade mixer, persistent speaker mixer +- **`backend/library/`** — Scan-specific cancellable context, suppressed orphan cleanup on cancel +- **`backend/queue/`** — `TrackLoader` interface gains `PreloadNext()`, queue exposes "peek next" capability + +**Database migrations** (current version = 5): +2 new tables (`smart_playlists`, `musicbrainz_cache`), most features use TOML config not DB. ### Critical Pitfalls -Top 5 from [PITFALLS.md](./PITFALLS.md), ordered by severity: +1. **Tag writing corrupts audio files (P1)** — `dhowden/tag` is read-only; new write libraries must use write-to-temp-then-rename. Block writes on currently-playing file (beep holds `*os.File` handle). Preserve all existing tag frames when editing; never create tags from scratch. -1. **Refactoring concurrency without tests creates invisible regressions** — Write characterization tests BEFORE fixing races. Fix `SetContext` first (lowest risk), Player last (most complex). The race detector is the oracle. -2. **Player deadlock from mutex + speaker lock ordering violation** — NEVER remove the `go p.onPlaybackFinished()` goroutine dispatch. NEVER refactor player lock code without drawing the full lock acquisition graph. Extract pure logic; leave lock-sensitive paths alone. -3. **FTS5 query consolidation breaks search ranking** — Write search tests BEFORE consolidating. Consolidate the JOIN clause only, not full queries. Verify `COALESCE` behavior is identical across all copies. -4. **Queue persistence migration loses queue state** — New persistence code must read old format. Test old-write → new-read compatibility. Keep full rewrite as fallback for complex operations. -5. **SQLite in-memory tests behave differently from file-based production** — Test helper must mirror production `NewDB()` exactly: same PRAGMAs, same migration sequence, `PRAGMA foreign_keys = ON`. Use `t.TempDir()` for file-based tests when WAL behavior matters. +2. **Gapless playback breaks lock ordering (P2)** — The existing `p.mu → speaker.Lock()` ordering assumes one streamer at a time. Pre-decoding a second track with crossfade means two concurrent streamer chains. Must suppress `onPlaybackFinished` callback during transitions, pre-decode in background goroutine, and close old `BufferedStreamer` only after crossfade completes. + +3. **Scan cancellation triggers orphan cleanup on partial data (P3)** — If walk is cancelled early, `existingPaths` sync.Map still contains valid files → orphan cleanup deletes them. **Must skip orphan cleanup on cancelled scans.** Check cancellation between DB writer batches, not mid-batch. + +4. **Plugin system crashes host app (P4)** — Go `plugin` package is Linux-only and fragile. For v1.1: JS-only frontend plugins (loaded via dynamic `import()`), Go hooks compiled-in (not dynamic). Wrap all plugin callbacks in `recover()`. Give plugins read-only DB access. + +5. **MusicBrainz rate limiting (P5)** — Strict 1 req/s enforced by IP ban. Must set meaningful User-Agent, cache responses in SQLite (24hr for searches, 7 days for entities), use `time.Ticker` rate limiter, handle 503 with exponential backoff. ## Implications for Roadmap -Based on dependency analysis across all four research files, with convergent recommendations: +Based on research, suggested phase structure: -### Phase 1: Correctness Fixes & Test Foundation +### Phase 1: Quick Wins — Scan Cancellation + Keyboard Shortcuts +**Rationale:** Lowest complexity, highest certainty, no new dependencies. Validates core integration patterns (context cancellation, config extension, event-driven sync) that every subsequent phase depends on. +**Delivers:** Cancellable library scans with graceful stop; configurable keyboard shortcuts with sensible defaults. +**Addresses:** Scan cancellation (all table stakes), keyboard shortcuts (all table stakes) +**Avoids:** P3 (skip orphan cleanup on cancel), P7 (capture phase listener, skip shortcuts on input focus), P12 (config backward compat — test with old config files) +**Stack:** No new dependencies. stdlib `context.WithCancel`, TOML config extension, Web `KeyboardEvent` API. -**Rationale:** Every other phase depends on either the concurrency fixes (to unblock `-race`-clean tests) or the test infrastructure (to safely refactor). This is the critical enabler. All four research files independently recommend this as the first step. +### Phase 2: Tag Editing +**Rationale:** Introduces the 3 new Go dependencies and validates the "write file → update DB → emit event → refresh frontend" pipeline. This pipeline is reused by smart playlists (DB updates trigger re-evaluation) and is a prerequisite for MusicBrainz becoming useful (users see MB data then want to apply it to their files). +**Delivers:** Single-track and batch tag editing for MP3 and FLAC files; cover art assignment; DB cascade updates; FTS5 re-indexing. +**Addresses:** Tag editing (all table stakes), cover art assignment +**Avoids:** P1 (write-to-temp-then-rename, block writes on playing file, preserve unedited frames), P9 (block tag edits during active scans) +**Stack:** n10v/id3v2 v2.1.4, go-flac/go-flac v2, go-flac/flacvorbis v2 -**Delivers:** Race-free `SetContext` in all packages, `startupErr` moved to struct, config permissions fixed, swallowed errors surfaced, in-memory SQLite test helper, event name codegen, extracted testable player logic. +### Phase 3: Smart Playlists +**Rationale:** Builds on validated DB infrastructure from Phase 2. Independent of audio pipeline. Medium complexity with well-understood patterns (SQL WHERE clause generation). Benefits from tag editing being complete (edited metadata changes smart playlist membership). +**Delivers:** Rule-based dynamic playlists with AND logic, configurable sort/limit, auto-refresh on library changes, sidebar integration. +**Addresses:** Smart playlists (all table stakes + random/limit differentiator) +**Avoids:** P6 (lazy evaluation — only re-evaluate on view, not on every library change; dedicated indexed queries, not VIEW-based full scans) +**Stack:** No new dependencies. Dynamic SQL with parameterized queries, new `smart_playlists` table (migration 6). -**Features addressed:** All "Must fix" table stakes items + test infrastructure. +### Phase 4: Gapless Playback + Crossfade +**Rationale:** Highest technical risk — must be built with full focus and thorough testing. No dependencies on other v1.1 features. The audio pipeline refactor (switching from per-track `speaker.Play()` to persistent `beep.Mixer`) is the biggest architectural change in v1.1. Build gapless first, then layer crossfade on top. +**Delivers:** Seamless track transitions; optional crossfade with configurable duration; pre-decoded next track for zero-gap playback. +**Addresses:** Gapless playback (table stakes), crossfade (table stakes), crossfade duration control +**Avoids:** P2 (pre-decode in background goroutine, suppress callback during transitions, close old BufferedStreamer after crossfade completes), P11 (always crossfade post-resample) +**Stack:** No new dependencies. beep.Mixer, beep.Seq, effects.Volume (all existing). -**Pitfalls avoided:** Pitfall 1 (concurrency without tests), Pitfall 2 (in-memory test divergence), Pitfall 5 (config migration failures via roundtrip test). +### Phase 5: MusicBrainz Browser +**Rationale:** First network feature — introduces HTTP client, caching, offline handling, rate limiting. Orthogonal to audio pipeline work. Can be developed independently. Becomes more valuable after tag editing exists (users can browse MB, then manually apply metadata). +**Delivers:** Artist search, discography browsing, release/track listing, response caching, offline-safe degradation. +**Addresses:** MusicBrainz browser (all table stakes + caching differentiator) +**Avoids:** P5 (1 req/s rate limiter, proper User-Agent, SQLite cache, exponential backoff on 503), P10 (separate cache table, display-only DTOs — never merge MB data into library schema), P13 (use bindings for data retrieval, events for notifications only) +**Stack:** No new dependencies. stdlib net/http + encoding/json, new `musicbrainz_cache` table (migration 7). -**Estimated items:** ~10 discrete changes, all LOW-MEDIUM effort individually. - -### Phase 2: Core Test Suite - -**Rationale:** With concurrency fixed and test infrastructure in place, write the safety net that protects all subsequent refactoring. Tests target the code AS IT IS (characterization tests), not as it will be after optimization. - -**Delivers:** Queue unit tests (~15-20), database/search tests (~10-15), config roundtrip tests (~8-10), player pure logic tests (~5-8), event parity test (1). Approximately 40-55 tests total. - -**Features addressed:** All test coverage items from FEATURES.md. - -**Pitfalls avoided:** Pitfall 1 (provides the safety net), Pitfall 4 (search tests before consolidation), Pitfall 6 (queue persistence tests before optimization). - -**Estimated effort:** HIGH — this is the largest phase by work volume, but it's the foundation for everything else. - -### Phase 3: SQL & Performance Optimization - -**Rationale:** With tests as a safety net, refactor the SQL layer and persistence. Schema changes (VIEW creation) should precede query pattern changes. Queue persistence optimization uses existing but unwired sqlc queries. - -**Delivers:** Deduplicated FTS5 queries via SQLite VIEW, incremental queue persistence for add/remove operations, SetQueue Phase 2 redundant lookup fix, scan warnings separated from fatal errors. - -**Features addressed:** FTS5 consolidation, queue persistence optimization, SetQueue Phase 2 fix, scan error separation. - -**Pitfalls avoided:** Pitfall 3 (FTS5 consolidation verified by Phase 2 tests), Pitfall 6 (queue persistence verified by Phase 2 tests). - -**Estimated effort:** MEDIUM — changes are well-scoped and verified by existing tests. - -### Phase 4: Frontend Performance & Polish - -**Rationale:** Frontend changes are independent of backend refactoring and lowest risk. The library store lazy loading is nearly zero-effort (removing code, not adding it). UI polish is last because it's the lowest priority for a consolidation milestone. - -**Delivers:** Lazy library loading (remove `eagerFetch()`), optimized re-renders with `repeat()` directive and stable keys, documentation of intentional exceptions (hand-crafted SQL, singleton store lifecycle). - -**Features addressed:** Library store lazy loading, frontend rendering optimization, documentation. - -**Pitfalls avoided:** Pitfall 5 (eager-to-lazy UX regression — mitigate by keeping eager for default view, audit all `getCached*` call sites). - -**Estimated effort:** LOW-MEDIUM — mostly removing code and CSS changes. +### Phase 6: Layout Customization + Plugin Foundation +**Rationale:** Meta-features that wrap all other features. Must come last because they need a stable API surface and complete component set. Layout customization is the prerequisite for plugin UI registration. Plugin system defines the extensibility API but ships as "foundation" (working loader + core API surface + example plugin). +**Delivers:** Section-based layout config (MusicBee-style); resizable panels with persistence; component registry; JS plugin loading; plugin API surface (events, player, queue, library); one example plugin. +**Addresses:** Layout customization (table stakes + section-based differentiator), plugin system (foundation — API definition, loading mechanism, core hooks) +**Avoids:** P4 (JS-only plugins, recover() wrappers, read-only DB for plugins, namespaced events), P8 (section-level operation not component-level, CSS Container Queries, explicit height for virtualized sections), P14 (extend existing stores where possible, component-local state for view-specific data) +**Stack:** No new dependencies. Lit customElements, dynamic import(), TOML config extension. ### Phase Ordering Rationale -- **Phase 1 → Phase 2:** You cannot write `-race`-clean tests without fixing the SetContext races first. Test infrastructure (`NewTestDB`) must exist before any DB-dependent tests. -- **Phase 2 → Phase 3:** Refactoring SQL and persistence without tests is the #1 pitfall identified by research. The tests characterize current behavior, then the refactoring is verified against them. -- **Phase 3 → Phase 4:** Frontend changes don't depend on backend refactoring, but doing them last means the backend API is stable. The SQLite VIEW from Phase 3 doesn't affect the frontend. -- **Within Phase 1:** SetContext fixes → test helper → event codegen (independent items, can be parallelized). -- **Within Phase 3:** SQL VIEW creation → query migration → queue persistence (schema before queries before consumers). +- **Dependency chain:** Scan cancel → validates context patterns used everywhere. Tag editing → validates file-write-DB-update-event pipeline. Smart playlists → uses validated DB patterns. Layout → provides component registry needed by plugins. Plugins → last because it depends on everything being stable. +- **Risk isolation:** Gapless playback (Phase 4) is the highest-risk change. Placing it mid-sequence means foundational patterns are proven and later features (MusicBrainz, layout, plugins) don't block on audio work. +- **Value delivery curve:** Phases 1-3 are low-to-medium risk and deliver immediate user-facing value. If the project stalls after Phase 3, users still get scan cancellation, keyboard shortcuts, tag editing, and smart playlists — a strong v1.1. +- **Feature grouping:** Each phase touches a distinct subsystem (config, files+DB, DB queries, audio pipeline, network, UI architecture), minimizing merge conflicts for parallel development. ### Research Flags -Phases likely needing deeper research during planning: -- **Phase 2 (Core Test Suite):** The queue test architecture needs careful design — mock player interface, test data seeding patterns, event verification strategy. `/gsd-research-phase` recommended for the queue test design. -- **Phase 3 (SQL Optimization):** sqlc's handling of SQLite VIEWs with FTS5 virtual tables needs validation. The VIEW concept is sound but edge cases in sqlc's SQLite parser are unknown. Quick validation needed before committing to VIEW approach. +**Phases likely needing deeper research during planning:** +- **Phase 4 (Gapless + Crossfade):** The beep library's Mixer/Seq composition for real-time crossfade is not well-documented beyond basic examples. Need to prototype the persistent-mixer architecture and validate lock ordering with two concurrent BufferedStreamers before committing to implementation approach. +- **Phase 6 (Plugin System):** The plugin API surface needs careful design — what's exposed, what's sandboxed, how errors are contained. No off-the-shelf solution fits; this is bespoke design work. Consider a spike/prototype before full implementation. -Phases with standard patterns (skip research-phase): -- **Phase 1 (Correctness Fixes):** All fixes are mechanical (add lock, move field, fix permissions). Well-documented Go patterns. -- **Phase 4 (Frontend):** Removing `eagerFetch()` is a one-line change. Lit `repeat()` directive is well-documented. +**Phases with standard patterns (skip deep research):** +- **Phase 1 (Scan Cancel + Shortcuts):** Well-documented Go context cancellation + standard web keyboard handling. The codebase already has the patterns. +- **Phase 2 (Tag Editing):** Tag writing libraries have clear APIs. The DB cascade is the main design work. +- **Phase 3 (Smart Playlists):** Dynamic SQL generation is a solved problem. Rules → WHERE clause mapping is straightforward. +- **Phase 5 (MusicBrainz):** REST API with excellent official documentation. Rate limiting patterns are standard. ## Confidence Assessment | Area | Confidence | Notes | |------|------------|-------| -| Stack | HIGH | All recommendations come from official docs (SQLite, Go stdlib, Lit, beep). Existing stack is correct; only PRAGMAs need addition. | -| Features | HIGH | All improvements grounded in direct codebase analysis + CONCERNS.md. Priority ordering validated by dependency analysis across all research files. | -| Architecture | HIGH | Patterns from Go stdlib, sqlc official docs. One MEDIUM area: sqlc VIEW support for SQLite needs validation. | -| Pitfalls | HIGH | All pitfalls derived from actual code paths (lock ordering, FTS5 duplication, persistence pattern). Recovery strategies are concrete. | +| Stack | HIGH | Only 3 new deps, all verified on pkg.go.dev. Existing stack covers 7/10 features with no additions. | +| Features | HIGH | Grounded in codebase analysis + established desktop music player patterns (foobar2000, MusicBee, Strawberry). | +| Architecture | HIGH | Derived from complete codebase read. Integration patterns validated against existing code structure. | +| Pitfalls | HIGH | 15 pitfalls identified with specific line-number references to codebase. Critical pitfalls have concrete prevention strategies. | **Overall confidence:** HIGH ### Gaps to Address -- **sqlc + SQLite VIEW + FTS5 compatibility:** MEDIUM confidence that sqlc correctly parses queries against VIEWs that JOIN with FTS5 virtual tables. Validate during Phase 3 planning — if it doesn't work, fall back to Go string constant for the JOIN clause. -- **`@lit-labs/signals` stability:** Used for signal-based reactivity in the frontend. Experimental API (v0.2.0) may change. Not blocking for consolidation but worth noting for future milestones. -- **Library scan test fixtures:** Testing the library scan requires audio file fixtures or a mock filesystem. `testing/fstest.MapFS` may not be sufficient for the metadata parsing paths. May need real (tiny) audio files as test fixtures. Validate during Phase 2 planning. -- **Lazy loading measurement:** The recommendation to remove `eagerFetch()` is based on architecture analysis, not profiling data. Before Phase 4, measure actual startup time with a large library to confirm lazy loading is beneficial. +- **OGG Vorbis tag writing:** No pure-Go solution exists. Deferred to v1.2+. Need to show "read-only" indicator in tag editor UI for OGG files. May need to revisit if user demand is high. +- **Play count tracking:** Required for advanced smart playlist rules ("most played", "never played") but not in current schema. Needs a schema migration and playback-completion hook. Defer to Phase 3 as an optional add-on. +- **Plugin security model:** The v1.1 foundation intentionally skips a permissions system. Plugins run with full API access. This is acceptable for "power user installs plugins manually" but needs a permissions model before any marketplace/discovery feature. +- **FLAC memory usage during tag writes:** `go-flac` reads entire files into memory. For 100MB+ FLAC files, this is significant. May need a streaming approach in the future, but acceptable for v1.1. +- **Crossfade timing accuracy:** Detecting "N seconds from track end" requires comparing `seeker.Position()` to `seeker.Len()` at the speaker sample rate. Accuracy depends on the polling interval. Need to prototype during Phase 4 to determine if a polling approach is sufficient or if a sample-counting approach is needed. ## Sources ### Primary (HIGH confidence) -- SQLite WAL documentation: https://www.sqlite.org/wal.html -- SQLite PRAGMA documentation: https://www.sqlite.org/pragma.html -- modernc.org/sqlite API: https://pkg.go.dev/modernc.org/sqlite@v1.46.1 -- Go race detector: https://go.dev/doc/articles/race_detector -- gopxl/beep wiki: https://github.com/gopxl/beep/wiki/Composing-and-controlling -- Lit rendering docs: https://lit.dev/docs/components/rendering/ -- Lit repeat directive: https://lit.dev/docs/templates/lists/#the-repeat-directive -- sqlc official docs: https://docs.sqlc.dev/en/stable/ -- Codebase analysis: `.planning/codebase/CONCERNS.md`, `.planning/codebase/STACK.md` -- Direct code inspection of all backend and frontend source files +- YellowJacket codebase: complete analysis of all Go packages and TypeScript sources (2026-03-06) +- n10v/id3v2: https://github.com/n10v/id3v2 — 359 stars, v2.1.4, MIT license, full ID3v2 read/write +- beep v2.1.1: https://pkg.go.dev/github.com/gopxl/beep/v2 — Mixer, Seq, Volume, Resample confirmed +- beep wiki: https://github.com/gopxl/beep/wiki/Composing-and-controlling — speaker.Lock(), Seq chaining, Ctrl pause +- MusicBrainz API: https://musicbrainz.org/doc/MusicBrainz_API — rate limiting, JSON format, entity types +- MusicBrainz rate limiting: https://musicbrainz.org/doc/MusicBrainz_API/Rate_Limiting — 1 req/s, User-Agent requirement +- dhowden/tag: confirmed read-only (no Save/Write methods in API) ### Secondary (MEDIUM confidence) -- beep speaker.Lock() behavior — inferred from beep wiki + codebase lock ordering comments -- sqlc VIEW support for SQLite — documented for PostgreSQL, inferred for SQLite -- Wails v2 binding generation and event system limitations — based on codebase patterns +- go-flac/go-flac: https://github.com/go-flac/go-flac — 44 stars, v2 available, Apache-2.0 +- go-flac/flacvorbis: https://github.com/go-flac/flacvorbis — 11 stars, v2 available, Apache-2.0 +- Desktop music player patterns: foobar2000, MusicBee, Strawberry, Deadbeef, Audacious (training data knowledge) +- michiwend/gomusicbrainz: https://github.com/michiwend/gomusicbrainz — 64 stars, confirmed unmaintained + +### Tertiary (LOW confidence) +- Plugin architecture recommendations: based on Go ecosystem analysis and desktop app patterns; no direct precedent for Wails plugin systems exists --- -*Research completed: 2026-02-27* +*Research completed: 2026-03-06* *Ready for roadmap: yes*