284 lines
21 KiB
Markdown
284 lines
21 KiB
Markdown
# Codebase Concerns
|
|
|
|
**Analysis Date:** 2026-02-26
|
|
|
|
## Tech Debt
|
|
|
|
**Hardcoded Speaker Configuration:**
|
|
- Issue: Speaker sample rate (44100) and buffer size (100ms) are hardcoded constants with no user configuration
|
|
- Files: `backend/player/player.go` line 104, line 127
|
|
- Impact: Users with high-resolution audio (96kHz, 192kHz) get resampled down to 44.1kHz. Users cannot tune buffer size for latency vs. stability tradeoff
|
|
- Fix approach: Add `AudioOutput` section to config TOML (`SampleRate`, `BufferSizeMs`). Plumb through to `InitSpeaker()` and `updateStreamers()` resample quality param (currently hardcoded `4` at line 308)
|
|
|
|
**Fixed Resample Quality:**
|
|
- Issue: Resample quality is hardcoded to `4` in `beep.Resample()` call
|
|
- Files: `backend/player/player.go` line 307-309
|
|
- Impact: No ability to trade CPU for quality. Low quality may produce audible artifacts on large sample rate deltas
|
|
- Fix approach: Make resample quality configurable via config, expose in settings UI. The TODO comment at line 307 acknowledges this
|
|
|
|
**Tag Writing Not Implemented:**
|
|
- Issue: Track details editing UI exists but save is a no-op
|
|
- Files: `frontend/src/components/track-details/track-details.ts` line 651
|
|
- Impact: Users see an edit interface that doesn't persist changes. Misleading UX
|
|
- Fix approach: Implement backend tag writing endpoint using a tag library (e.g. `github.com/dhowden/tag` already in deps supports reading; writing may need additional library). Gate the save button behind a "tag writing supported" check
|
|
|
|
**HTML Template Component Incomplete:**
|
|
- Issue: The `struct2html` templ component has a TODO for supporting more types
|
|
- Files: `pkg/templcomp/struct2html_templ.go` line 242
|
|
- Impact: Config page form generation may not handle all field types correctly
|
|
- Fix approach: Extend the type switch to cover missing types (maps, nested structs, etc.)
|
|
|
|
**Package-Level `startupErr` Variable:**
|
|
- Issue: `startupErr` is a package-level mutable variable used to communicate startup failures between `OnStartup` and `OnDomReady`
|
|
- Files: `backend/app.go` line 134
|
|
- Impact: Not thread-safe if Wails calls these lifecycle methods concurrently. Also makes testing difficult
|
|
- Fix approach: Move to a field on `YellowJacketApp` struct, protected by the struct's lifecycle guarantees
|
|
|
|
## Code Quality
|
|
|
|
**Large Frontend Components:**
|
|
- Issue: Several Lit components exceed 1000+ lines, combining rendering, state management, event handling, drag-and-drop, context menus, and search filtering
|
|
- Files:
|
|
- `frontend/src/components/playlist-view/playlist-view.ts` (2669 lines)
|
|
- `frontend/src/components/cover-grid/cover-grid.ts` (2092 lines)
|
|
- `frontend/src/components/track-list/track-list.ts` (1875 lines)
|
|
- `frontend/src/components/config-page/config-page.ts` (1464 lines)
|
|
- `frontend/src/components/queue-panel/queue-panel.ts` (1424 lines)
|
|
- Impact: Difficult to reason about, test in isolation, or modify without regressions. High coupling between rendering and business logic
|
|
- Fix approach: Extract reusable behaviors into additional controllers (the project already uses `SelectionController`, `ContextMenuController`, etc.). Consider splitting rendering into sub-components
|
|
|
|
**Large Backend Files:**
|
|
- Issue: `backend/playlist/playlist.go` (1778 lines) and `backend/library/library.go` (1328 lines) handle too many responsibilities
|
|
- Files: `backend/playlist/playlist.go`, `backend/library/library.go`
|
|
- Impact: Hard to navigate; mixing CRUD, M3U8 file management, phantom resolution, and search in a single file
|
|
- Fix approach: `playlist.go` already has some splitting (m3u.go, match.go, favorites.go). Consider further extraction: phantom resolution into `phantom.go`, M3U file management is already split. Library could extract `saveAudioFile`/`updateAudioFileMetadata`/`processMetadata` into a dedicated `import.go` file
|
|
|
|
**Duplicated FTS Search Query:**
|
|
- Issue: The same complex FTS5 JOIN query pattern (audio_files + recordings + artist_credit + release_group_recordings + release_groups) is repeated in `SearchFTS`, `SearchFTSByFilename`, `SearchFTSTracks`, `RebuildSearchIndex`, and `migration2BasenameAndFTS`
|
|
- Files: `backend/database/search.go` lines 34-57, 92-116, 232-274, 168-188; `backend/database/database.go` lines 287-311
|
|
- Impact: Changes to the schema require updating 5+ copies of essentially the same JOIN pattern. Risk of them diverging
|
|
- Fix approach: Extract the common JOIN clause into a constant or query builder helper. Alternatively, consolidate into fewer sqlc-generated queries
|
|
|
|
**Raw SQL in Persistence Layer:**
|
|
- Issue: Queue persistence and search use hand-crafted SQL with string concatenation for batch operations (`lookupChunk`, `insertTrackBatch`) instead of sqlc-generated queries
|
|
- Files: `backend/queue/persistence.go` lines 56-73, 186-203; `backend/database/search.go`
|
|
- Impact: These queries bypass sqlc's type-safety guarantees. The `fmt.Sprintf` pattern for IN clauses is safe (only `?` placeholders are interpolated) but diverges from the project's pattern of using generated queries
|
|
- Fix approach: Consider using sqlc's `sqlc.slice()` feature or a query builder for batch operations. Alternatively, document these as intentional exceptions
|
|
|
|
## Error Handling Gaps
|
|
|
|
**Swallowed Errors in App Lifecycle Callbacks:**
|
|
- Issue: MPRIS callbacks in `app.go` discard errors from `Pause()` and `Seek()` with `_ =`
|
|
- Files: `backend/app.go` lines 183, 186, 191, 195
|
|
- Impact: If pause or seek fails from OS media controls, the failure is invisible to the user and to logs
|
|
- Fix approach: Log errors at minimum. Consider emitting a frontend notification for user-visible failures
|
|
|
|
**Silently Swallowed Artist Credit Link Error:**
|
|
- Issue: `CreateArtistCreditArtist` result and error are both discarded with `_, _`
|
|
- Files: `backend/library/library.go` line 1092
|
|
- Impact: If the link creation fails for a non-duplicate reason, the data model is silently incomplete
|
|
- Fix approach: Check error; ignore only `UNIQUE constraint` violations (which are expected for idempotent upserts), log all others
|
|
|
|
**Library Scan Error Accumulation:**
|
|
- Issue: `Scan()` accumulates errors via `errors.Join` but individual file failures don't stop the scan — which is correct behavior — but the accumulated `scanErr` is returned alongside valid metrics, and callers may not distinguish "scan completed with warnings" from "scan failed"
|
|
- Files: `backend/library/library.go` lines 216-218, 310-320, 427-430
|
|
- Impact: Callers cannot differentiate between partial success and total failure
|
|
- Fix approach: Consider separating scan warnings from fatal scan errors. Return warnings in metrics, fatal errors as the error return
|
|
|
|
**Config File Permissions:**
|
|
- Issue: Config file is written with `0o666` permissions
|
|
- Files: `backend/config/config.go` line 152
|
|
- Impact: On multi-user systems, any user can read/write the config file. While this is a desktop app, it's not best practice
|
|
- Fix approach: Use `0o644` or `0o600` for user-only read/write
|
|
|
|
## Performance Concerns
|
|
|
|
**Eager Full-Library Fetch on Startup:**
|
|
- Issue: `libraryStore.eagerFetch()` calls `GetAllTracks()`, `GetAllAlbums()`, `GetAllArtists()`, `GetAllGenres()` simultaneously on construction
|
|
- Files: `frontend/src/store/library-store.ts` lines 300-305
|
|
- Impact: For large libraries (50k+ tracks), this loads all track data into memory at once. Each call triggers a full table scan with multiple JOINs
|
|
- Fix approach: Consider lazy loading only the active view's data, or implement pagination. The `GetAllTracks` query with full metadata joins is particularly expensive for large libraries
|
|
|
|
**Full Queue Re-persist on Every Mutation:**
|
|
- Issue: `commitMutation()` calls `persistTracks()` which does `DELETE FROM queue_tracks` + batch INSERT for the entire queue on every add/remove/move operation
|
|
- Files: `backend/queue/persistence.go` lines 118-178; `backend/queue/queue.go` line 1157
|
|
- Impact: For a queue with thousands of tracks, every single track add/remove triggers a full table rewrite. This is O(n) for every mutation
|
|
- Fix approach: Use incremental persistence (INSERT/DELETE individual rows) for add/remove operations. Reserve full rewrite for SetQueue and restore
|
|
|
|
**SetQueue Phase 2 Re-lookups All Tracks:**
|
|
- Issue: `resolveRemainingTracks` re-fetches metadata for ALL file paths including those already resolved in Phase 1
|
|
- Files: `backend/queue/queue.go` lines 258-311
|
|
- Impact: For large albums/playlists, this doubles the DB work for the initial batch
|
|
- Fix approach: Pass the already-resolved metadata from Phase 1 to Phase 2, only lookup the remaining paths
|
|
|
|
**Entity Cache Never Evicted During Scan:**
|
|
- Issue: The `entityCache` in library scanning grows unbounded during a scan - it accumulates every artist, album, genre, and cover art seen
|
|
- Files: `backend/library/library.go` lines 41-61
|
|
- Impact: For very large libraries with thousands of unique artists/albums, this could consume significant memory. However, since it's only held for the duration of a scan and reduces DB round-trips, this is an acceptable tradeoff for most libraries
|
|
- Fix approach: Low priority. Could add an LRU eviction policy if memory becomes an issue with extremely large libraries
|
|
|
|
## Security Considerations
|
|
|
|
**File Path Handling:**
|
|
- Risk: Library scan uses `filepath.Join(basePath, path)` where `path` comes from `fs.WalkDir` which should be safe, but playlist import accepts user-provided file paths (`ImportPlaylist`, `AddTracksToPlaylist`)
|
|
- Files: `backend/playlist/playlist.go` lines 677-784, 442-484; `backend/library/library.go` line 247
|
|
- Current mitigation: File paths come from Wails file dialogs (OS-level) and are validated by checking file existence. sqlc parameterized queries prevent SQL injection
|
|
- Recommendations: Consider adding path traversal validation (ensure paths don't escape expected directories). Validate that playlist import paths resolve within the library directory
|
|
|
|
**SQL Injection Protection:**
|
|
- Risk: Most queries use sqlc-generated parameterized queries, but hand-crafted SQL exists in search and queue persistence
|
|
- Files: `backend/queue/persistence.go` lines 64-73, 195-198; `backend/database/search.go` lines 34-58, 92-116
|
|
- Current mitigation: All hand-crafted queries use `?` placeholders with separate args — no string interpolation of user values into SQL
|
|
- Recommendations: The `fmt.Sprintf` in `lookupChunk` only interpolates placeholder strings (`"?"` literals), not user data. This is safe but should be documented with a comment explaining why
|
|
|
|
**Config Data Logged:**
|
|
- Risk: Config struct is attached to the logger context at construction time
|
|
- Files: `backend/config/config.go` line 46
|
|
- Current mitigation: Config currently contains no secrets (file paths, theme settings, window dimensions)
|
|
- Recommendations: If secrets are ever added to config (API keys, auth tokens), the logger attachment must be removed or filtered
|
|
|
|
## Fragile Areas
|
|
|
|
**Event Name Synchronization:**
|
|
- Files: `backend/events/events.go`, `frontend/src/events.ts`
|
|
- Why fragile: Event names must match exactly between Go and TypeScript. There is no compile-time or runtime verification that they match. A typo in either file silently breaks communication
|
|
- Safe modification: Always update both files simultaneously. The AGENTS.md documents this requirement
|
|
- Test coverage: No automated test verifies event name parity
|
|
|
|
**Player Lock Ordering:**
|
|
- Files: `backend/player/player.go` lines 31-39
|
|
- Why fragile: The player has two locks (its own `sync.Mutex` and the global `speaker.Lock()`) with a documented ordering requirement: "always acquire p.mu BEFORE speaker.Lock()". The `onPlaybackFinished` callback runs on a goroutine to avoid holding both locks simultaneously
|
|
- Safe modification: Never call `speaker.Lock()` while holding `p.mu` in a code path that could block. The `go p.onPlaybackFinished()` pattern in the beep callback (line 351) is critical — removing the goroutine dispatch would deadlock
|
|
- Test coverage: No test validates the lock ordering. The integration test requires hardware
|
|
|
|
**Two-Phase Queue Initialization:**
|
|
- Files: `backend/queue/queue.go` lines 152-251
|
|
- Why fragile: `SetQueue` uses a two-phase approach with generation counters to handle concurrent calls. The background goroutine (`resolveRemainingTracks`) must check the generation counter under the lock to avoid overwriting newer state
|
|
- Safe modification: Always increment `setQueueGen` before starting background work. Always check the counter both before and after acquiring the lock
|
|
- Test coverage: No unit test for concurrent SetQueue calls
|
|
|
|
**Player SetContext Double Lock:**
|
|
- Files: `backend/player/player.go` lines 163-171
|
|
- Why fragile: `SetContext` acquires and releases `p.mu` twice in succession. Between the two lock acquisitions, another goroutine could modify state
|
|
- Safe modification: Consider combining into a single lock acquisition, or document why the two-phase approach is intentional (it appears to be separating the context set from the state restore for clarity)
|
|
- Test coverage: Integration test only
|
|
|
|
**Config TOML Serialization Roundtrip:**
|
|
- Files: `backend/config/config.go` lines 100-139, 142-160
|
|
- Why fragile: `Load()` applies defaults, then decodes TOML over them, then validates. If a new config field is added without a proper default, existing config files will have the zero value. The `applyDefaults()` runs after decode which could overwrite valid zero values
|
|
- Safe modification: Always add defaults in `applyDefaults()` for new fields. Test with an empty config file
|
|
|
|
## Missing Features
|
|
|
|
**No Graceful Scan Cancellation:**
|
|
- Problem: Library scan cannot be cancelled by the user once started
|
|
- Files: `backend/library/library.go` lines 166-540
|
|
- Blocks: Users with large libraries cannot abort a scan that's taking too long. The `l.ctx.Done()` checks exist but depend on the Wails context which is only cancelled on app shutdown
|
|
- Fix approach: Add a separate cancellation context that can be triggered from the frontend
|
|
|
|
**No Database Connection Pooling/Health Check:**
|
|
- Problem: The database connection is opened once at startup with no health checking or reconnection logic
|
|
- Files: `backend/database/database.go` lines 35-136
|
|
- Blocks: If the SQLite file becomes corrupted or the disk fills up, errors propagate to every component with no recovery path
|
|
- Fix approach: Add a health check method and consider periodic PRAGMA integrity_check for dev builds
|
|
|
|
**No Cross-Platform Media Controls:**
|
|
- Problem: Media controls only work on Linux (MPRIS). macOS and Windows get a no-op stub
|
|
- Files: `backend/mediacontrols/mpris_linux.go`, `backend/mediacontrols/stub.go`
|
|
- Blocks: macOS users cannot control playback from the media keys overlay or Control Center
|
|
- Fix approach: Implement `NSMPRemoteCommandCenter` for macOS, `SystemMediaTransportControls` for Windows
|
|
|
|
## Test Coverage Gaps
|
|
|
|
**No Queue Unit Tests:**
|
|
- What's not tested: Queue operations (SetQueue, AddTrack, RemoveTrack, Next, Previous, shuffle, repeat modes, persistence)
|
|
- Files: `backend/queue/queue.go`, `backend/queue/navigation.go`, `backend/queue/persistence.go`, `backend/queue/handlers.go`
|
|
- Risk: The queue is central to playback. Bugs in index tracking, shuffle order, or persistence could cause tracks to skip, repeat incorrectly, or lose the queue on restart
|
|
- Priority: High
|
|
|
|
**No Library Service Unit Tests:**
|
|
- What's not tested: Library scan logic, metadata processing, entity cache behavior, batch commit logic, orphan cleanup
|
|
- Files: `backend/library/library.go`, `backend/library/rescan.go`, `backend/library/coverart.go`
|
|
- Risk: Scan bugs could silently drop tracks, create duplicate entities, or fail to clean up orphans
|
|
- Priority: High
|
|
|
|
**No Database Layer Tests:**
|
|
- What's not tested: Search index operations (FTS5 queries), migration logic, transaction handling
|
|
- Files: `backend/database/search.go`, `backend/database/database.go`
|
|
- Risk: FTS5 query edge cases (special characters, empty queries, very long queries) and migration failures on existing databases
|
|
- Priority: Medium
|
|
|
|
**No Config Tests:**
|
|
- What's not tested: Config load/save roundtrip, validation, default application, migration from older config formats
|
|
- Files: `backend/config/config.go`
|
|
- Risk: Config corruption or silent loss of settings on upgrade
|
|
- Priority: Medium
|
|
|
|
**Player Tests Require Hardware:**
|
|
- What's not tested: All player tests require an audio device and are skipped in CI
|
|
- Files: `backend/player/player_test.go` line 21
|
|
- Risk: Player regressions are only caught manually. The volume conversion, streamer chain, and state persistence logic could all be tested without hardware
|
|
- Priority: Medium — extract pure logic (volume math, state serialization) into testable functions
|
|
|
|
**No Frontend Tests:**
|
|
- What's not tested: All TypeScript/Lit components, stores, and controllers
|
|
- Files: `frontend/src/` (entire directory)
|
|
- Risk: Frontend regressions in event handling, state synchronization, search filtering, drag-and-drop, and selection logic
|
|
- Priority: Medium — the backend is the source of truth, but frontend-only logic (search ranking, column sorting, selection controller) could have unit tests
|
|
|
|
## Concurrency Concerns
|
|
|
|
**Queue Context Set Without Lock:**
|
|
- Issue: `Queue.SetContext()` sets `q.ctx` without holding `q.mu`, while `q.ctx` is read by emit methods that are called under `q.mu`
|
|
- Files: `backend/queue/queue.go` lines 134-136
|
|
- Impact: Technically a data race on `q.ctx` if SetContext is called concurrently with emit methods. In practice, SetContext is called once during startup before any other queue operations
|
|
- Fix approach: Acquire `q.mu` in SetContext for correctness
|
|
|
|
**Library Fields Not Protected:**
|
|
- Issue: `Library` struct fields (`ctx`, `conf`, `rescanHooks`) are set via setter methods without any synchronization
|
|
- Files: `backend/library/library.go` lines 78-84, 88-90, 120-123
|
|
- Impact: If `SetContext`, `SetRescanHooks`, or config updates occur concurrently with a scan, there could be data races. In practice, these are called during the single-threaded startup phase
|
|
- Fix approach: Low priority — document the "set during startup only" contract, or add a mutex if the initialization order becomes less predictable
|
|
|
|
**Playlist Service Context Race:**
|
|
- Issue: `playlist.Service` has a `ctx` field set by `SetContext()` without synchronization, read by `emitEvent()` and all methods
|
|
- Files: `backend/playlist/playlist.go` lines 98-104, 130-133, 1169-1178
|
|
- Impact: Same pattern as Queue — safe in practice due to startup ordering but technically a race
|
|
- Fix approach: Same as Queue — acquire lock or document contract
|
|
|
|
## Frontend Concerns
|
|
|
|
**No Event Listener Cleanup:**
|
|
- Issue: Singleton stores (`playerStore`, `queueStore`, `libraryStore`) register `EventsOn` listeners in their constructors but never unregister them
|
|
- Files: `frontend/src/store/player-store.ts` lines 54-71, `frontend/src/store/queue-store.ts` lines 65-105, `frontend/src/store/library-store.ts` line 51
|
|
- Impact: As singletons that live for the app lifetime, this is acceptable — they never need cleanup. However, the Wails `EventsOn` API returns a cancel function that is never captured. If the architecture ever changes to non-singleton stores, this would leak
|
|
- Fix approach: Low priority — capture the cancel functions for documentation purposes even if they're never called
|
|
|
|
**Library Store Potential Memory Pressure:**
|
|
- Issue: `libraryStore` caches the entire track, album, artist, and genre lists in memory simultaneously
|
|
- Files: `frontend/src/store/library-store.ts` lines 29-32
|
|
- Impact: For a library with 100k+ tracks, this could be tens of MB of JavaScript objects. The eager fetch on construction (`eagerFetch()`) means all four datasets are loaded simultaneously
|
|
- Fix approach: Consider lazy loading per-view and releasing data for inactive views, or implementing virtual scrolling data providers that don't require holding the full dataset
|
|
|
|
**Queue Store Delta Application Trusts Backend:**
|
|
- Issue: The `applyTracksDelta` method in `QueueStore` applies backend-sent delta operations without validation. If the frontend state diverges from the backend (e.g. missed event), the delta application produces incorrect state
|
|
- Files: `frontend/src/store/queue-store.ts` lines 107-171
|
|
- Impact: Could cause visual glitches where the queue panel shows incorrect tracks or indices. The full-state `QueueChanged` event acts as a periodic correction mechanism
|
|
- Fix approach: Consider adding a sequence number or hash to detect state divergence and trigger a full re-sync
|
|
|
|
## Dependencies at Risk
|
|
|
|
**Wails v2 Framework Lock-in:**
|
|
- Risk: Wails v2 uses WebView2 (Windows), WebKit2 (Linux), WKWebView (macOS). The project requires `-tags webkit2_41` for Linux builds. Wails v3 is in active development with breaking API changes
|
|
- Impact: Migration to Wails v3 will require significant refactoring of the lifecycle management (`OnStartup`, `OnDomReady`, `OnShutdown`), event system, and binding registration
|
|
- Migration plan: Monitor Wails v3 stability. The event-based architecture and clean separation of concerns make migration more feasible than a tightly coupled approach
|
|
|
|
**beep Audio Library:**
|
|
- Risk: The `gopxl/beep/v2` library handles all audio decoding and playback. It wraps platform-specific audio output (oto) and codec libraries. The speaker is initialized with global state (`speaker.Init`, `speaker.Lock`)
|
|
- Impact: The global speaker lock creates an implicit coupling between all audio operations. If beep has bugs in seeking or resampling, workarounds are limited
|
|
- Migration plan: The `metadata.DecodeFile()` abstraction and `TrackLoader` interface provide some insulation. A replacement would require reimplementing the streamer chain
|
|
|
|
---
|
|
|
|
*Concerns audit: 2026-02-26*
|