docs: complete project research for consolidation milestone
This commit is contained in:
@@ -0,0 +1,754 @@
|
||||
# Architecture Research: Refactoring Patterns for YellowJacket Consolidation
|
||||
|
||||
**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)
|
||||
|
||||
## Issue 1: Two-Phase Initialization Race Conditions
|
||||
|
||||
### Current Problem
|
||||
|
||||
Six components use a `SetContext(ctx context.Context)` pattern where the Wails runtime context is stored on a struct field without synchronization:
|
||||
|
||||
```go
|
||||
// queue/queue.go:134 — no lock
|
||||
func (q *Queue) SetContext(ctx context.Context) {
|
||||
q.ctx = ctx
|
||||
}
|
||||
|
||||
// library/library.go:120 — no lock, also calls registerEventHandlers()
|
||||
func (l *Library) SetContext(ctx context.Context) {
|
||||
l.ctx = ctx
|
||||
l.registerEventHandlers()
|
||||
}
|
||||
|
||||
// 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()
|
||||
}
|
||||
```
|
||||
|
||||
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.
|
||||
|
||||
### Recommended Approach: Mutex-Guarded SetContext
|
||||
|
||||
**Do NOT use `sync.Once` or `atomic.Value`.** These are the wrong tools because:
|
||||
|
||||
- `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.
|
||||
|
||||
**Instead, hold the existing mutex through the entire SetContext operation:**
|
||||
|
||||
```go
|
||||
// queue/queue.go — recommended fix
|
||||
func (q *Queue) SetContext(ctx context.Context) {
|
||||
q.mu.Lock()
|
||||
defer q.mu.Unlock()
|
||||
|
||||
q.ctx = ctx
|
||||
}
|
||||
|
||||
// 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()
|
||||
}
|
||||
```
|
||||
|
||||
For **Library** and **Playlist**, which don't have a mutex because they currently have no concurrent access pattern, add one:
|
||||
|
||||
```go
|
||||
type Library struct {
|
||||
mu sync.Mutex // protects ctx and conf
|
||||
ctx context.Context
|
||||
// ... rest unchanged
|
||||
}
|
||||
|
||||
func (l *Library) SetContext(ctx context.Context) {
|
||||
l.mu.Lock()
|
||||
defer l.mu.Unlock()
|
||||
|
||||
l.ctx = ctx
|
||||
l.registerEventHandlers()
|
||||
}
|
||||
```
|
||||
|
||||
**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.
|
||||
|
||||
### `startupErr` Package-Level Variable
|
||||
|
||||
Move to a field on `YellowJacketApp`:
|
||||
|
||||
```go
|
||||
type YellowJacketApp struct {
|
||||
// ... existing fields ...
|
||||
startupErr error // set in OnStartup, checked in OnDomReady
|
||||
}
|
||||
```
|
||||
|
||||
This is safe because Wails guarantees `OnStartup` completes before `OnDomReady` runs — they are sequentially called lifecycle hooks, not concurrent.
|
||||
|
||||
### Risk Assessment
|
||||
|
||||
| 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 |
|
||||
|
||||
### 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`:
|
||||
|
||||
```go
|
||||
// cmd/genevents/main.go
|
||||
package main
|
||||
|
||||
import (
|
||||
"go/ast"
|
||||
"go/parser"
|
||||
"go/token"
|
||||
"os"
|
||||
"text/template"
|
||||
)
|
||||
|
||||
const tmpl = `// Code generated by cmd/genevents. DO NOT EDIT.
|
||||
|
||||
export const Events = {
|
||||
{{- range .}}
|
||||
{{.Name}}: "{{.Value}}",
|
||||
{{- end}}
|
||||
} as const;
|
||||
|
||||
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)
|
||||
}
|
||||
```
|
||||
|
||||
Wire into the existing `go generate ./...` pipeline via a directive in `events.go`:
|
||||
|
||||
```go
|
||||
//go:generate go run ../../cmd/genevents/main.go
|
||||
package events
|
||||
```
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
**Build verification step:** Add a `make` target or pre-commit hook check:
|
||||
|
||||
```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.
|
||||
|
||||
---
|
||||
|
||||
## Issue 3: Store Architecture for Large Datasets
|
||||
|
||||
### Current Problem
|
||||
|
||||
`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.
|
||||
|
||||
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.
|
||||
|
||||
### Recommended Approach: Lazy Loading by Active View
|
||||
|
||||
The fix is surgical — the infrastructure is already there:
|
||||
|
||||
**Step 1: Remove `eagerFetch()` from constructor.** Change the constructor to only set up event listeners:
|
||||
|
||||
```typescript
|
||||
constructor() {
|
||||
EventsOn(Events.LibraryScanComplete, () => {
|
||||
this.invalidate();
|
||||
});
|
||||
this.loadCoverSize();
|
||||
// Remove: this.eagerFetch();
|
||||
}
|
||||
```
|
||||
|
||||
**Step 2: Make `invalidate()` only clear caches, not re-fetch:**
|
||||
|
||||
```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();
|
||||
}
|
||||
```
|
||||
|
||||
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.
|
||||
|
||||
**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).
|
||||
|
||||
---
|
||||
|
||||
## Issue 4: Queue Persistence — Incremental Updates
|
||||
|
||||
### Current Problem
|
||||
|
||||
`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.
|
||||
|
||||
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.
|
||||
|
||||
### Recommended Approach: Operation-Specific Persistence
|
||||
|
||||
Replace the single `persistTracks()` call with operation-specific methods:
|
||||
|
||||
**For AddTrack/AddTracks:** INSERT only the new tracks.
|
||||
|
||||
```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)
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**For RemoveTrack/RemoveTracks:** DELETE specific rows + shift positions.
|
||||
|
||||
```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()
|
||||
}
|
||||
```
|
||||
|
||||
**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.
|
||||
|
||||
**For SetQueue and Clear:** Keep the existing DELETE ALL + batch INSERT — these are full replacement operations by definition.
|
||||
|
||||
**Refactored `commitMutation`:**
|
||||
|
||||
```go
|
||||
type mutationKind int
|
||||
const (
|
||||
mutationFull mutationKind = iota // SetQueue, Clear
|
||||
mutationAdd // AddTrack, AddTracks
|
||||
mutationRemove // RemoveTrack, RemoveTracks
|
||||
mutationReorder // MoveQueueTracks, InsertNext*
|
||||
)
|
||||
|
||||
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.
|
||||
|
||||
---
|
||||
|
||||
## Issue 5: SQL Query Consolidation — FTS5 JOIN Pattern
|
||||
|
||||
### Current Problem
|
||||
|
||||
The same JOIN pattern (audio_files → recordings → artist_credit → release_group_recordings → release_groups) appears in:
|
||||
|
||||
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:**
|
||||
|
||||
```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)
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### Mocking Strategy
|
||||
|
||||
**Use real in-memory SQLite, not mocked interfaces.** Reasons:
|
||||
|
||||
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)
|
||||
|
||||
**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
|
||||
|
||||
### 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
|
||||
```
|
||||
|
||||
### Phase Ordering Rationale
|
||||
|
||||
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.
|
||||
|
||||
2. **SetContext fixes (1c) before queue tests (2a)** because the race conditions in SetContext would cause flaky test failures under `-race`.
|
||||
|
||||
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.
|
||||
|
||||
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.
|
||||
|
||||
---
|
||||
|
||||
## Anti-Patterns to Avoid
|
||||
|
||||
### Anti-Pattern 1: Interface-Heavy Mocking
|
||||
|
||||
**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).
|
||||
|
||||
### Anti-Pattern 2: Premature Abstraction of 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.
|
||||
|
||||
### Anti-Pattern 3: Global Event Bus Replacement
|
||||
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
## 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**
|
||||
|
||||
---
|
||||
*Architecture research for: YellowJacket consolidation milestone*
|
||||
*Researched: 2026-02-27*
|
||||
@@ -0,0 +1,362 @@
|
||||
# Feature Research: Quality Improvements
|
||||
|
||||
**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`.
|
||||
|
||||
---
|
||||
|
||||
### Table Stakes (Must Fix — Codebase Is Unreliable Without These)
|
||||
|
||||
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.*
|
||||
|
||||
---
|
||||
|
||||
### Differentiators (Raises Quality Significantly)
|
||||
|
||||
These improvements go beyond "not broken" to "genuinely well-engineered." They improve performance, maintainability, and user experience noticeably.
|
||||
|
||||
| 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 |
|
||||
|
||||
#### FTS5 Query Consolidation Details
|
||||
|
||||
**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
|
||||
|
||||
**Recommended approach:** Create a SQL view for the common JOIN:
|
||||
|
||||
```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;
|
||||
```
|
||||
|
||||
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.
|
||||
|
||||
**Alternative:** Extract the JOIN clause as a Go string constant and compose queries from it. Less elegant but simpler to implement.
|
||||
|
||||
**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.*
|
||||
|
||||
#### Queue Persistence Optimization Details
|
||||
|
||||
**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)
|
||||
```
|
||||
|
||||
The sqlc queries `InsertQueueTrack`, `RemoveQueueTrack`, `ShiftQueuePositionsDown`, `ShiftQueuePositionsUp` already exist but aren't used by `commitMutation()`. Wire them up for single-track operations.
|
||||
|
||||
*Confidence: HIGH — the individual queries already exist in sqlc.*
|
||||
|
||||
#### Library Store Lazy Loading Details
|
||||
|
||||
**Current:** Constructor calls `eagerFetch()` → 4 parallel Wails binding calls → 4 full table scans with JOINs → all data in JS memory.
|
||||
|
||||
**Improved pattern:**
|
||||
```typescript
|
||||
class LibraryStore {
|
||||
// Load on first access, not constructor
|
||||
async getTracks(): Promise<library.Track[]> {
|
||||
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
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
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.
|
||||
|
||||
**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.
|
||||
|
||||
*Confidence: HIGH — the lazy loading infrastructure already exists.*
|
||||
|
||||
#### Frontend Performance Details
|
||||
|
||||
**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.*
|
||||
|
||||
---
|
||||
|
||||
### Anti-Features (Things to Deliberately NOT Do During Refactoring)
|
||||
|
||||
| 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`. |
|
||||
|
||||
---
|
||||
|
||||
## 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)
|
||||
```
|
||||
|
||||
### 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.
|
||||
|
||||
---
|
||||
|
||||
## Prioritization
|
||||
|
||||
### Phase 1: Correctness & Test Foundation (Do First)
|
||||
|
||||
Fixes known bugs and establishes the test infrastructure that makes everything else safe.
|
||||
|
||||
- [ ] 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
|
||||
|
||||
### Phase 2: SQL & Performance Foundations (Do Second)
|
||||
|
||||
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*
|
||||
@@ -0,0 +1,288 @@
|
||||
# Pitfalls Research
|
||||
|
||||
**Domain:** Go/Wails/SQLite Desktop Music Player — Consolidation & Refactoring
|
||||
**Researched:** 2026-02-27
|
||||
**Confidence:** HIGH (based on codebase analysis + established Go/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.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall 2: SQLite In-Memory Tests Behave Differently From File-Based Production DB
|
||||
|
||||
**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
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
**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
|
||||
|
||||
**Phase to address:**
|
||||
First phase — the test infrastructure setup. `NewTestDB()` must be correct before any database tests are written.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall 3: Deadlock From Player mutex + speaker.Lock() Ordering Violation
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
**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
|
||||
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall 4: FTS5 Query Consolidation Breaks Search Ranking or Returns
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
**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
|
||||
|
||||
**Phase to address:**
|
||||
Database/code quality phase. Write FTS5 search tests first, then consolidate.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall 5: Eager-to-Lazy Library Loading Creates Visible UX Regression
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
**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
|
||||
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall 6: Queue Persistence Migration Loses Queue State
|
||||
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall 7: Wails Binding Regeneration Silently Breaks Frontend After Go Struct Changes
|
||||
|
||||
**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.
|
||||
|
||||
**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`).
|
||||
|
||||
**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.
|
||||
|
||||
**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
|
||||
|
||||
**Phase to address:**
|
||||
Every phase that touches Go structs used in events. Add a validation check (build script or test) early.
|
||||
|
||||
## 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. |
|
||||
|
||||
## Integration Gotchas
|
||||
|
||||
| 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. |
|
||||
|
||||
## Performance Traps
|
||||
|
||||
| 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. |
|
||||
|
||||
## UX Pitfalls
|
||||
|
||||
| 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
|
||||
|
||||
- [ ] **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
|
||||
|
||||
## Recovery Strategies
|
||||
|
||||
| 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. |
|
||||
|
||||
## Pitfall-to-Phase Mapping
|
||||
|
||||
| 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. |
|
||||
|
||||
## 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)
|
||||
|
||||
---
|
||||
*Pitfalls research for: YellowJacket consolidation milestone*
|
||||
*Researched: 2026-02-27*
|
||||
@@ -0,0 +1,657 @@
|
||||
# Stack Research: Consolidation Patterns & Tools
|
||||
|
||||
**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)
|
||||
|
||||
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.
|
||||
|
||||
---
|
||||
|
||||
## 1. Go Concurrency Safety — Priority: CRITICAL
|
||||
|
||||
**Confidence:** HIGH — based on Go standard library docs, race detector behavior, and codebase analysis.
|
||||
|
||||
### The Core Problem
|
||||
|
||||
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:
|
||||
|
||||
```go
|
||||
// BEFORE (race):
|
||||
func (q *Queue) SetContext(ctx context.Context) {
|
||||
q.ctx = ctx // ← no lock, but q.ctx is read under q.mu elsewhere
|
||||
}
|
||||
|
||||
// AFTER (correct):
|
||||
func (q *Queue) SetContext(ctx context.Context) {
|
||||
q.mu.Lock()
|
||||
defer q.mu.Unlock()
|
||||
q.ctx = ctx
|
||||
}
|
||||
```
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
### Pattern: Player Double-Lock Fix
|
||||
|
||||
The player's `SetContext` acquires and releases the mutex twice in succession:
|
||||
|
||||
```go
|
||||
// BEFORE (window between locks):
|
||||
func (p *Player) SetContext(ctx context.Context) {
|
||||
p.mu.Lock()
|
||||
p.ctx = ctx
|
||||
p.mu.Unlock()
|
||||
|
||||
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()
|
||||
}
|
||||
```
|
||||
|
||||
**Why:** Between the two lock acquisitions, another goroutine can modify state. The combined lock makes the set-context-and-restore atomic.
|
||||
|
||||
### Pattern: Lock Ordering Documentation
|
||||
|
||||
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`.
|
||||
|
||||
**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).
|
||||
|
||||
```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
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
### Testing Pattern: Race Detector as Test Oracle
|
||||
|
||||
```bash
|
||||
# Already in Makefile — verify this is the exact command:
|
||||
make test # → go test -tags webkit2_41 -race -count=1 -timeout 120s ./...
|
||||
```
|
||||
|
||||
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:
|
||||
|
||||
```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. |
|
||||
|
||||
---
|
||||
|
||||
## 2. SQLite WAL Mode Optimization — Priority: HIGH
|
||||
|
||||
**Confidence:** HIGH — based on SQLite official docs (sqlite.org/wal.html), modernc.org/sqlite driver docs, and codebase analysis.
|
||||
|
||||
### Current Setup Analysis
|
||||
|
||||
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**)
|
||||
|
||||
### Missing PRAGMAs to Add
|
||||
|
||||
```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
|
||||
}
|
||||
```
|
||||
|
||||
**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:
|
||||
<lit-virtualizer
|
||||
.items=${this.tracks}
|
||||
.keyFunction=${(track: Track) => track.filePath}
|
||||
.renderItem=${(track: Track) => html`...`}
|
||||
></lit-virtualizer>
|
||||
```
|
||||
|
||||
**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 |
|
||||
|
||||
---
|
||||
|
||||
## 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)
|
||||
|
||||
---
|
||||
|
||||
*Stack research for: YellowJacket consolidation milestone*
|
||||
*Researched: 2026-02-27*
|
||||
@@ -0,0 +1,189 @@
|
||||
# Project Research Summary
|
||||
|
||||
**Project:** YellowJacket — Desktop Music Player Consolidation
|
||||
**Domain:** Go/Wails/Lit desktop application — codebase quality & reliability improvement
|
||||
**Researched:** 2026-02-27
|
||||
**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.
|
||||
|
||||
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 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.
|
||||
|
||||
## Key Findings
|
||||
|
||||
### Recommended Stack
|
||||
|
||||
The existing stack is correct. No changes needed. See [STACK.md](./STACK.md) for full details.
|
||||
|
||||
**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
|
||||
|
||||
**Critical version note:** Match modernc.org/libc version exactly per upstream warning when updating modernc.org/sqlite.
|
||||
|
||||
### Expected Features
|
||||
|
||||
This is a consolidation milestone — "features" are quality improvements, not user-facing functionality. See [FEATURES.md](./FEATURES.md) for full details.
|
||||
|
||||
**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 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)
|
||||
|
||||
### 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.
|
||||
|
||||
**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.
|
||||
|
||||
### Critical Pitfalls
|
||||
|
||||
Top 5 from [PITFALLS.md](./PITFALLS.md), ordered by severity:
|
||||
|
||||
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.
|
||||
|
||||
## Implications for Roadmap
|
||||
|
||||
Based on dependency analysis across all four research files, with convergent recommendations:
|
||||
|
||||
### Phase 1: Correctness Fixes & Test Foundation
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
**Features addressed:** All "Must fix" table stakes items + test infrastructure.
|
||||
|
||||
**Pitfalls avoided:** Pitfall 1 (concurrency without tests), Pitfall 2 (in-memory test divergence), Pitfall 5 (config migration failures via roundtrip test).
|
||||
|
||||
**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 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).
|
||||
|
||||
### 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 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.
|
||||
|
||||
## 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. |
|
||||
|
||||
**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.
|
||||
|
||||
## 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
|
||||
|
||||
### 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
|
||||
|
||||
---
|
||||
*Research completed: 2026-02-27*
|
||||
*Ready for roadmap: yes*
|
||||
Reference in New Issue
Block a user