docs(06): create phase plan — 3 plans for SQL consolidation, event codegen, and SAFETY comments
This commit is contained in:
@@ -96,7 +96,11 @@ Plans:
|
||||
2. A code generator reads Go event constants from `backend/events/events.go` and produces `frontend/src/events.ts`, wired into `go generate` and the pre-commit hook — adding an event in Go without regenerating TypeScript fails the hook
|
||||
3. Queue batch lookups in `persistence.go` use `sqlc.slice()` for IN clauses where sqlc supports it, replacing `fmt.Sprintf` placeholder construction
|
||||
4. Every hand-crafted SQL statement that intentionally bypasses sqlc has a `// SAFETY:` comment explaining why (batch INSERT, dynamic IN clauses, etc.)
|
||||
**Plans:** TBD
|
||||
**Plans:** 3 plans
|
||||
Plans:
|
||||
- [ ] 06-01-PLAN.md — Create track_metadata VIEW and consolidate search queries
|
||||
- [ ] 06-02-PLAN.md — Event codegen tool (Go→TypeScript) and pre-commit hook wiring
|
||||
- [ ] 06-03-PLAN.md — Migrate lookupChunk to sqlc.slice() and add SAFETY comments to all hand-crafted SQL
|
||||
|
||||
### Phase 7: Backend Performance
|
||||
**Goal:** Queue mutations and library loading are fast — single-track queue changes are O(1) instead of O(n), and the library doesn't block startup with a full data fetch
|
||||
@@ -128,7 +132,7 @@ Plans:
|
||||
| 3. Test Infrastructure | 0/1 | Planned | — |
|
||||
| 4. Queue, Config & Player Tests | 0/2 | Planned | — |
|
||||
| 5. Database & Library Tests | 0/2 | Planned | — |
|
||||
| 6. SQL Consolidation & Code Quality | 0/? | Not started | — |
|
||||
| 6. SQL Consolidation & Code Quality | 0/3 | Planned | — |
|
||||
| 7. Backend Performance | 0/? | Not started | — |
|
||||
| 8. Frontend Performance & UX | 0/? | Not started | — |
|
||||
|
||||
|
||||
@@ -0,0 +1,241 @@
|
||||
---
|
||||
phase: 06-sql-consolidation-code-quality
|
||||
plan: 01
|
||||
type: execute
|
||||
wave: 1
|
||||
depends_on: []
|
||||
files_modified:
|
||||
- backend/database/database.go
|
||||
- backend/database/search.go
|
||||
- backend/database/sql/schemas/track_metadata_view.sql
|
||||
- backend/database/sql/sqlcgen/models.go
|
||||
autonomous: true
|
||||
requirements: [QUAL-01]
|
||||
|
||||
must_haves:
|
||||
truths:
|
||||
- "All FTS5 search queries (SearchFTS, SearchFTSByFilename, SearchFTSTracks) use the track_metadata VIEW instead of inline 5-table JOINs"
|
||||
- "RebuildSearchIndex SELECTs from track_metadata VIEW instead of duplicating the JOIN"
|
||||
- "Migration 4 creates the track_metadata VIEW for existing databases"
|
||||
- "sqlc generate succeeds with the VIEW schema file and produces updated models"
|
||||
- "Existing FTS5 search tests (15 tests) pass unchanged after VIEW consolidation"
|
||||
artifacts:
|
||||
- path: "backend/database/sql/schemas/track_metadata_view.sql"
|
||||
provides: "VIEW definition for sqlc schema awareness"
|
||||
contains: "CREATE VIEW IF NOT EXISTS track_metadata"
|
||||
- path: "backend/database/database.go"
|
||||
provides: "Migration 4 creating VIEW for existing databases"
|
||||
contains: "migration4TrackMetadataView"
|
||||
- path: "backend/database/search.go"
|
||||
provides: "Consolidated search queries using VIEW"
|
||||
contains: "track_metadata"
|
||||
key_links:
|
||||
- from: "backend/database/search.go"
|
||||
to: "track_metadata VIEW"
|
||||
via: "JOIN track_metadata tm ON tm.id = si.rowid"
|
||||
pattern: "JOIN track_metadata"
|
||||
- from: "backend/database/database.go"
|
||||
to: "track_metadata VIEW"
|
||||
via: "migration 4 CREATE VIEW"
|
||||
pattern: "CREATE VIEW IF NOT EXISTS track_metadata"
|
||||
---
|
||||
|
||||
<objective>
|
||||
Consolidate the duplicated 5-table FTS5 JOIN pattern into a single SQLite VIEW named `track_metadata`, and update all search queries to use it.
|
||||
|
||||
Purpose: Eliminate 4+ copies of the same complex JOIN across search.go and database.go. A single VIEW is the source of truth for audio file metadata JOINs — changes to the schema only need updating in one place.
|
||||
|
||||
Output: Migration 4 (VIEW creation), sqlc schema file, consolidated search.go queries, updated sqlc-generated code.
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
@/home/caleb/.config/opencode/get-shit-done/workflows/execute-plan.md
|
||||
@/home/caleb/.config/opencode/get-shit-done/templates/summary.md
|
||||
</execution_context>
|
||||
|
||||
<context>
|
||||
@.planning/PROJECT.md
|
||||
@.planning/ROADMAP.md
|
||||
@.planning/STATE.md
|
||||
@.planning/phases/06-sql-consolidation-code-quality/06-RESEARCH.md
|
||||
|
||||
@backend/database/database.go
|
||||
@backend/database/search.go
|
||||
@backend/database/sql/schemas/
|
||||
@backend/database/sqlc.yaml
|
||||
|
||||
<interfaces>
|
||||
<!-- Key types and contracts the executor needs. -->
|
||||
|
||||
From backend/database/database.go:
|
||||
- Migrations are Go functions registered in a slice, applied sequentially by PRAGMA user_version
|
||||
- Pattern: `migration2BasenameAndFTS`, `migration3UniqueArtistCreditArtist` — each bumps user_version
|
||||
- Current highest migration: 3 (user_version=3)
|
||||
- `//go:generate go tool sqlc generate` directive at line 21
|
||||
|
||||
From backend/database/search.go:
|
||||
- `func (d *DB) SearchFTS(query string, limit int) ([]SearchResult, error)` — line 22
|
||||
- `func (d *DB) SearchFTSByFilename(query string, limit int) ([]SearchResult, error)` — line 72
|
||||
- `func (d *DB) RebuildSearchIndex() error` — line 161
|
||||
- `func (d *DB) SearchFTSTracks(query string, limit int) ([]SearchTrackResult, error)` — line 222
|
||||
- All 4 functions contain inline 5-table JOINs (audio_files → recordings → artist_credit → release_group_recordings subquery → release_groups)
|
||||
|
||||
From backend/database/sql/schemas/ directory:
|
||||
- Schema files sorted alphabetically; sqlc processes them in filesystem order
|
||||
- Tables: artist_credit.sql, artists.sql, audio_files.sql, cover_art.sql, file_types.sql, genres.sql, recordings.sql, release_group_recordings.sql, release_groups.sql, etc.
|
||||
- `track_metadata_view.sql` will sort after all table schemas (t > all existing prefixes)
|
||||
</interfaces>
|
||||
</context>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 1: Create track_metadata VIEW schema and migration</name>
|
||||
<files>
|
||||
backend/database/sql/schemas/track_metadata_view.sql
|
||||
backend/database/database.go
|
||||
</files>
|
||||
<action>
|
||||
1. Create `backend/database/sql/schemas/track_metadata_view.sql` with the VIEW definition:
|
||||
```sql
|
||||
CREATE VIEW IF NOT EXISTS track_metadata AS
|
||||
SELECT
|
||||
af.id,
|
||||
af.file_path,
|
||||
af.length_milliseconds,
|
||||
COALESCE(r.name, '') AS title,
|
||||
COALESCE(ac.text, '') AS artist_name,
|
||||
r.track_number,
|
||||
r.disc_number,
|
||||
COALESCE(rg.name, '') AS album,
|
||||
CAST(COALESCE(
|
||||
(SELECT GROUP_CONCAT(g.name, '||')
|
||||
FROM recording_genres rg_sub
|
||||
JOIN genres g ON rg_sub.genre_id = g.id
|
||||
WHERE rg_sub.recording_id = r.id),
|
||||
''
|
||||
) AS TEXT) AS genre,
|
||||
COALESCE(r.year, 0) AS year,
|
||||
COALESCE(r.composer, '') AS composer,
|
||||
COALESCE(ft.extension, '') AS file_type,
|
||||
af.sample_rate,
|
||||
af.bit_depth,
|
||||
af.channels,
|
||||
af.bitrate,
|
||||
af.file_size
|
||||
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
|
||||
LEFT JOIN file_types ft ON af.file_type_id = ft.id;
|
||||
```
|
||||
|
||||
2. In `backend/database/database.go`, add migration 4 (`migration4TrackMetadataView`):
|
||||
- The migration function should execute `CREATE VIEW IF NOT EXISTS track_metadata AS ...` (same SQL as the schema file)
|
||||
- Register it in the migrations slice after migration 3
|
||||
- Follow the existing migration function pattern (takes `*sql.DB` and `context.Context`, returns `error`)
|
||||
|
||||
3. Run `go tool sqlc generate` from `backend/database/` to regenerate code with VIEW awareness.
|
||||
|
||||
4. **CRITICAL:** Do NOT change `migration2BasenameAndFTS` to use the VIEW — migration 2 runs before migration 4 for databases upgrading from version 1. The inline JOIN in migration 2 must stay as-is.
|
||||
|
||||
5. Verify sqlc generate succeeds without errors.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd backend/database && go tool sqlc generate && echo "sqlc OK"</automated>
|
||||
</verify>
|
||||
<done>
|
||||
- `track_metadata_view.sql` exists in schemas directory with the VIEW definition
|
||||
- Migration 4 registered in database.go, creates the VIEW for existing databases
|
||||
- `sqlc generate` succeeds and recognizes the VIEW
|
||||
- migration2 code is unchanged (still uses inline JOIN)
|
||||
</done>
|
||||
</task>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 2: Consolidate search queries to use track_metadata VIEW</name>
|
||||
<files>
|
||||
backend/database/search.go
|
||||
</files>
|
||||
<action>
|
||||
Update all 4 search functions in `search.go` to use the `track_metadata` VIEW instead of inline JOINs:
|
||||
|
||||
1. **SearchFTS** (line ~22): Replace the inline 5-table JOIN with:
|
||||
```sql
|
||||
SELECT tm.file_path, tm.length_milliseconds, tm.title, tm.artist_name, tm.album
|
||||
FROM search_index si
|
||||
JOIN track_metadata tm ON tm.id = si.rowid
|
||||
WHERE search_index MATCH ?
|
||||
ORDER BY rank
|
||||
LIMIT ?
|
||||
```
|
||||
Only select the 5 columns the function actually uses — SQLite optimizes away unused VIEW columns.
|
||||
|
||||
2. **SearchFTSByFilename** (line ~72): Same pattern as SearchFTS but with the filename-specific FTS query logic. Replace the inline JOIN with `JOIN track_metadata tm ON tm.id = si.rowid`. Keep the same column selection.
|
||||
|
||||
3. **SearchFTSTracks** (line ~222): Replace the inline 6-table JOIN (includes file_types) with the VIEW. The VIEW already includes `file_type` (from the file_types JOIN), so this becomes simpler. Select the columns needed by `SearchTrackResult`: file_path, length_milliseconds, title, artist_name, album, track_number, disc_number, genre, year, composer, file_type, sample_rate, bit_depth, channels, bitrate, file_size.
|
||||
|
||||
4. **RebuildSearchIndex** (line ~161): Replace the inline JOIN with:
|
||||
```sql
|
||||
INSERT INTO search_index(rowid, file_path, title, artist, album)
|
||||
SELECT id, file_path, title, artist_name, album
|
||||
FROM track_metadata
|
||||
```
|
||||
|
||||
**Preserve:** All FTS5 MATCH syntax, ORDER BY rank, LIMIT clauses, error handling, row scanning, and function signatures remain identical. Only the FROM/JOIN clauses change.
|
||||
|
||||
**Do NOT touch:** `InsertSearchIndex`, `DeleteSearchIndex`, `ClearSearchIndex` — these are single-row FTS5 operations that don't use JOINs.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd backend && go test -tags webkit2_41 -race -count=1 -timeout 60s ./database/...</automated>
|
||||
</verify>
|
||||
<done>
|
||||
- SearchFTS, SearchFTSByFilename, SearchFTSTracks, and RebuildSearchIndex all use `track_metadata` VIEW
|
||||
- No inline 5-table JOIN patterns remain in search.go (except in comments)
|
||||
- All 15 existing FTS5 search tests pass with -race
|
||||
- Function signatures unchanged — callers are unaffected
|
||||
</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<verification>
|
||||
```bash
|
||||
# 1. Verify sqlc generates cleanly
|
||||
cd backend/database && go tool sqlc generate
|
||||
|
||||
# 2. Verify all database tests pass (15 search tests + migrations)
|
||||
cd backend && go test -tags webkit2_41 -race -count=1 -timeout 60s ./database/...
|
||||
|
||||
# 3. Verify no inline JOIN duplication remains in search.go
|
||||
grep -c "LEFT JOIN recordings" backend/database/search.go # Should be 0
|
||||
|
||||
# 4. Verify VIEW is referenced
|
||||
grep -c "track_metadata" backend/database/search.go # Should be 4+
|
||||
|
||||
# 5. Verify migration2 is unchanged
|
||||
grep "LEFT JOIN recordings" backend/database/database.go # Should still exist (migration2 only)
|
||||
|
||||
# 6. Full build check
|
||||
go build -tags webkit2_41 ./...
|
||||
```
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
- The duplicated 5-table JOIN pattern is eliminated from search.go (0 copies remain)
|
||||
- All search queries use the `track_metadata` VIEW
|
||||
- Migration 4 creates the VIEW for existing databases
|
||||
- sqlc schema file enables future sqlc queries against the VIEW
|
||||
- All 15 existing database tests pass with -race
|
||||
- Full project builds without errors
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
After completion, create `.planning/phases/06-sql-consolidation-code-quality/06-01-SUMMARY.md`
|
||||
</output>
|
||||
@@ -0,0 +1,253 @@
|
||||
---
|
||||
phase: 06-sql-consolidation-code-quality
|
||||
plan: 02
|
||||
type: execute
|
||||
wave: 1
|
||||
depends_on: []
|
||||
files_modified:
|
||||
- backend/events/events.go
|
||||
- backend/events/cmd/genevents/main.go
|
||||
- frontend/src/events.ts
|
||||
- lefthook.yml
|
||||
autonomous: true
|
||||
requirements: [QUAL-02]
|
||||
|
||||
must_haves:
|
||||
truths:
|
||||
- "Running `go generate ./backend/events/...` produces frontend/src/events.ts that exactly matches the Go constants"
|
||||
- "The generated events.ts includes LibraryConfigChanged (currently missing from hand-maintained TS file)"
|
||||
- "The codegen-check pre-commit hook detects stale events.ts and fails"
|
||||
- "Output is deterministic — running the generator twice produces identical output"
|
||||
artifacts:
|
||||
- path: "backend/events/cmd/genevents/main.go"
|
||||
provides: "Go→TypeScript event constant generator"
|
||||
contains: "go/ast"
|
||||
- path: "backend/events/events.go"
|
||||
provides: "go:generate directive for event codegen"
|
||||
contains: "go:generate"
|
||||
- path: "frontend/src/events.ts"
|
||||
provides: "Generated TypeScript event constants"
|
||||
contains: "LibraryConfigChanged"
|
||||
key_links:
|
||||
- from: "backend/events/events.go"
|
||||
to: "frontend/src/events.ts"
|
||||
via: "go:generate directive running genevents"
|
||||
pattern: "go:generate go run"
|
||||
- from: "lefthook.yml"
|
||||
to: "go generate"
|
||||
via: "codegen-check pre-commit hook"
|
||||
pattern: "go generate"
|
||||
---
|
||||
|
||||
<objective>
|
||||
Build a Go code generator that reads event constants from `backend/events/events.go` using `go/ast` and produces `frontend/src/events.ts`, then wire it into `go generate` and the pre-commit hook.
|
||||
|
||||
Purpose: Eliminate manual synchronization of event names between Go and TypeScript. The generator automatically catches drift (like the missing `LibraryConfigChanged`) and the pre-commit hook prevents stale files from being committed.
|
||||
|
||||
Output: Generator tool, `//go:generate` directive, updated events.ts with missing constant, working codegen-check hook.
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
@/home/caleb/.config/opencode/get-shit-done/workflows/execute-plan.md
|
||||
@/home/caleb/.config/opencode/get-shit-done/templates/summary.md
|
||||
</execution_context>
|
||||
|
||||
<context>
|
||||
@.planning/PROJECT.md
|
||||
@.planning/ROADMAP.md
|
||||
@.planning/STATE.md
|
||||
@.planning/phases/06-sql-consolidation-code-quality/06-RESEARCH.md
|
||||
|
||||
@backend/events/events.go
|
||||
@frontend/src/events.ts
|
||||
@lefthook.yml
|
||||
|
||||
<interfaces>
|
||||
<!-- Current event constants for reference -->
|
||||
|
||||
From backend/events/events.go (21 constants in 5 groups):
|
||||
```go
|
||||
// Playback events (backend → frontend push).
|
||||
const (
|
||||
PlaybackStateChanged = "PlaybackStateChanged"
|
||||
PlaybackFinished = "PlaybackFinished"
|
||||
TrackChanged = "TrackChanged"
|
||||
SeekFailed = "SeekFailed"
|
||||
VolumeChanged = "VolumeChanged"
|
||||
)
|
||||
// Queue events (backend → frontend push).
|
||||
const (
|
||||
QueueChanged = "QueueChanged"
|
||||
QueueIndexChanged = "QueueIndexChanged"
|
||||
QueueModeChanged = "QueueModeChanged"
|
||||
QueueTracksModified = "QueueTracksModified"
|
||||
)
|
||||
// Config events.
|
||||
const (
|
||||
LibraryConfigChanged = "LibraryConfigChanged" // <-- MISSING from TS
|
||||
ThemeConfigChanged = "ThemeConfigChanged"
|
||||
TrackListConfigChanged = "TrackListConfigChanged"
|
||||
FavoritesConfigChanged = "FavoritesConfigChanged"
|
||||
)
|
||||
// Playlist events.
|
||||
const (
|
||||
PlaylistCreated = "PlaylistCreated"
|
||||
PlaylistDeleted = "PlaylistDeleted"
|
||||
PlaylistRenamed = "PlaylistRenamed"
|
||||
PlaylistTracksChanged = "PlaylistTracksChanged"
|
||||
PlaylistsRestored = "PlaylistsRestored"
|
||||
DefaultPlaylistChanged = "DefaultPlaylistChanged"
|
||||
)
|
||||
// Library events.
|
||||
const (
|
||||
LibraryScanStarted = "LibraryScanStarted"
|
||||
LibraryScanComplete = "LibraryScanComplete"
|
||||
)
|
||||
```
|
||||
|
||||
From frontend/src/events.ts (20 constants — missing LibraryConfigChanged):
|
||||
- Format: `export const Events = { ... } as const;`
|
||||
- Followed by: `export type EventName = (typeof Events)[keyof typeof Events];`
|
||||
- Comment groups match Go groups (Playback, Queue, Playlist, Config, Library)
|
||||
|
||||
From lefthook.yml:
|
||||
- codegen-check hook runs `go generate ./...` then checks `git diff --name-only`
|
||||
- Hook currently hangs per STATE.md but research shows `go generate ./...` now completes in <1s
|
||||
|
||||
Existing go:generate directives:
|
||||
- `backend/app.go:4` — `//go:generate go tool templ generate`
|
||||
- `backend/database/database.go:21` — `//go:generate go tool sqlc generate`
|
||||
</interfaces>
|
||||
</context>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 1: Create event codegen tool</name>
|
||||
<files>
|
||||
backend/events/cmd/genevents/main.go
|
||||
backend/events/events.go
|
||||
</files>
|
||||
<action>
|
||||
1. Create `backend/events/cmd/genevents/main.go` — a standalone Go program (package main) that:
|
||||
- Uses `go/ast`, `go/parser`, `go/token` to parse `events.go` in the same directory as the source
|
||||
- Accepts a `-source` flag (path to events.go, default: the events.go file relative to the generator location) and an `-output` flag (path to output .ts file)
|
||||
- Walks the AST in declaration order (NOT map iteration — deterministic output is critical)
|
||||
- For each `const` block: extracts the doc comment above the block (e.g., "// Playback events (backend → frontend push).") and each constant name + string value
|
||||
- Generates TypeScript output matching the current `events.ts` format exactly:
|
||||
```typescript
|
||||
// Code generated by genevents from backend/events/events.go. DO NOT EDIT.
|
||||
|
||||
export const Events = {
|
||||
// Playback events (backend → frontend push)
|
||||
PlaybackStateChanged: "PlaybackStateChanged",
|
||||
...
|
||||
} as const;
|
||||
|
||||
export type EventName = (typeof Events)[keyof typeof Events];
|
||||
```
|
||||
- Preserves comment group separation with blank lines between groups
|
||||
- Strips the trailing period from Go doc comments (Go convention) for TypeScript comments
|
||||
- Writes output atomically (write to temp file, then rename)
|
||||
|
||||
2. Add `//go:generate` directive to `backend/events/events.go`:
|
||||
```go
|
||||
//go:generate go run ./cmd/genevents -source events.go -output ../../frontend/src/events.ts
|
||||
```
|
||||
Place it after the package doc comment and before the first const block. Use a relative path from the events package directory to the frontend output.
|
||||
|
||||
3. Run `go generate ./backend/events/...` and verify the output matches the expected format.
|
||||
|
||||
4. Verify the generated events.ts now includes `LibraryConfigChanged` (the constant missing from the hand-maintained file).
|
||||
|
||||
**Key constraint:** AST iteration must be in source declaration order (iterate `f.Decls` directly, NOT collect into a map). This ensures deterministic output so the codegen-check hook doesn't produce false diffs.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>go generate ./backend/events/... && diff <(cat frontend/src/events.ts) <(go run ./backend/events/cmd/genevents -source backend/events/events.go -output /dev/stdout) && echo "Deterministic OK" && grep -q "LibraryConfigChanged" frontend/src/events.ts && echo "Missing constant fixed"</automated>
|
||||
</verify>
|
||||
<done>
|
||||
- Generator exists at backend/events/cmd/genevents/main.go
|
||||
- `//go:generate` directive added to events.go
|
||||
- Running `go generate ./backend/events/...` produces valid events.ts
|
||||
- Output includes all 21 constants (including LibraryConfigChanged)
|
||||
- Output is deterministic (running twice produces identical files)
|
||||
- Comment groups match Go source ordering
|
||||
</done>
|
||||
</task>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 2: Wire codegen-check pre-commit hook</name>
|
||||
<files>
|
||||
lefthook.yml
|
||||
</files>
|
||||
<action>
|
||||
1. The existing `codegen-check` hook in `lefthook.yml` already runs `go generate ./...` and diffs. Per research, `go generate ./...` now completes in <1 second (previous hanging appears resolved). The hook structure should work as-is with the new event generator wired in.
|
||||
|
||||
2. Test the hook end-to-end:
|
||||
- Run `go generate ./...` and verify it completes quickly (<5 seconds)
|
||||
- Verify no unstaged changes exist after generation (all generated code is up-to-date)
|
||||
- Manually introduce a drift: add a test constant to events.go, verify `go generate` updates events.ts, then verify the hook would detect the diff
|
||||
|
||||
3. If the hook still hangs (unlikely per research): narrow the `codegen-check` glob to only trigger on event-related files, or split into a separate event-specific check. Update lefthook.yml accordingly.
|
||||
|
||||
4. Run the full pre-commit hook to verify all hooks pass:
|
||||
```bash
|
||||
LEFTHOOK=1 lefthook run pre-commit
|
||||
```
|
||||
Note: If the hook takes >10 seconds, investigate and optimize. Expected: <5s total.
|
||||
|
||||
5. Clean up any test changes (remove test constant if added).
|
||||
|
||||
**Important:** The hook runs `go generate ./...` which triggers ALL generators (templ, sqlc, events). This is the correct behavior — it ensures all generated code is fresh. The <1s completion time makes this acceptable.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>go generate ./... && test -z "$(git diff --name-only)" && echo "codegen-check would pass"</automated>
|
||||
</verify>
|
||||
<done>
|
||||
- `go generate ./...` completes in <5 seconds
|
||||
- codegen-check hook detects stale events.ts (adding Go constant without regenerating TS fails the hook)
|
||||
- All existing pre-commit hooks still pass
|
||||
- No leftover test changes in the working tree
|
||||
</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<verification>
|
||||
```bash
|
||||
# 1. Generator produces valid output
|
||||
go generate ./backend/events/...
|
||||
|
||||
# 2. Output includes all 21 constants
|
||||
grep -c ":" frontend/src/events.ts # Should be 21+ (constants + type line)
|
||||
|
||||
# 3. LibraryConfigChanged is present
|
||||
grep "LibraryConfigChanged" frontend/src/events.ts
|
||||
|
||||
# 4. Deterministic output
|
||||
go generate ./backend/events/...
|
||||
git diff --name-only # Should be empty (no changes on second run)
|
||||
|
||||
# 5. Full generate works
|
||||
go generate ./...
|
||||
|
||||
# 6. Frontend typecheck passes with new events.ts
|
||||
cd frontend && ./node_modules/.bin/tsc --noEmit
|
||||
|
||||
# 7. Full build
|
||||
go build -tags webkit2_41 ./...
|
||||
```
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
- Event codegen tool parses Go constants and generates matching TypeScript
|
||||
- LibraryConfigChanged gap is automatically fixed
|
||||
- `go generate` directive wired into events.go
|
||||
- codegen-check hook works end-to-end (detects drift, passes when clean)
|
||||
- Frontend TypeScript compiles with generated events.ts
|
||||
- Output is deterministic across runs
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
After completion, create `.planning/phases/06-sql-consolidation-code-quality/06-02-SUMMARY.md`
|
||||
</output>
|
||||
@@ -0,0 +1,317 @@
|
||||
---
|
||||
phase: 06-sql-consolidation-code-quality
|
||||
plan: 03
|
||||
type: execute
|
||||
wave: 2
|
||||
depends_on: [06-01]
|
||||
files_modified:
|
||||
- backend/database/sql/queries/audio_files.sql
|
||||
- backend/database/sql/sqlcgen/audio_files.sql.go
|
||||
- backend/database/sql/sqlcgen/models.go
|
||||
- backend/queue/persistence.go
|
||||
- backend/database/search.go
|
||||
- backend/library/library.go
|
||||
- backend/library/rescan.go
|
||||
autonomous: true
|
||||
requirements: [QUAL-03, QUAL-04]
|
||||
|
||||
must_haves:
|
||||
truths:
|
||||
- "lookupChunk no longer uses fmt.Sprintf for IN clause construction — it calls a sqlc-generated query via the track_metadata VIEW"
|
||||
- "Every hand-crafted SQL statement that bypasses sqlc has a // SAFETY: comment with two parts: why sqlc can't handle it AND what makes it safe"
|
||||
- "All 12 identified hand-crafted SQL statements have SAFETY comments"
|
||||
- "Queue tests and database tests pass unchanged after the migration"
|
||||
artifacts:
|
||||
- path: "backend/database/sql/queries/audio_files.sql"
|
||||
provides: "sqlc query for batch track metadata lookup"
|
||||
contains: "LookupTrackMetaByPaths"
|
||||
- path: "backend/queue/persistence.go"
|
||||
provides: "Updated lookupChunk using sqlc-generated query"
|
||||
contains: "SAFETY"
|
||||
- path: "backend/database/search.go"
|
||||
provides: "SAFETY comments on all FTS5 queries"
|
||||
contains: "SAFETY"
|
||||
- path: "backend/library/library.go"
|
||||
provides: "SAFETY comments on FTS5 insert/delete operations"
|
||||
contains: "SAFETY"
|
||||
- path: "backend/library/rescan.go"
|
||||
provides: "SAFETY comments on FTS5 delete operation"
|
||||
contains: "SAFETY"
|
||||
key_links:
|
||||
- from: "backend/queue/persistence.go"
|
||||
to: "backend/database/sql/sqlcgen/"
|
||||
via: "sqlc-generated LookupTrackMetaByPaths query"
|
||||
pattern: "LookupTrackMetaByPaths"
|
||||
- from: "backend/database/sql/queries/audio_files.sql"
|
||||
to: "track_metadata VIEW"
|
||||
via: "SELECT FROM track_metadata WHERE file_path IN (sqlc.slice)"
|
||||
pattern: "sqlc.slice"
|
||||
---
|
||||
|
||||
<objective>
|
||||
Migrate the queue's `lookupChunk` from hand-crafted SQL with `fmt.Sprintf` to a sqlc-generated query using the `track_metadata` VIEW and `sqlc.slice()`, then add `// SAFETY:` comments to all remaining hand-crafted SQL statements.
|
||||
|
||||
Purpose: Replace the only hand-crafted SQL that CAN be migrated to sqlc (lookupChunk), and document all intentional exceptions so future maintainers understand why each hand-crafted statement exists.
|
||||
|
||||
Output: sqlc query file, regenerated code, updated persistence.go, SAFETY comments on all 12 hand-crafted SQL statements.
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
@/home/caleb/.config/opencode/get-shit-done/workflows/execute-plan.md
|
||||
@/home/caleb/.config/opencode/get-shit-done/templates/summary.md
|
||||
</execution_context>
|
||||
|
||||
<context>
|
||||
@.planning/PROJECT.md
|
||||
@.planning/ROADMAP.md
|
||||
@.planning/STATE.md
|
||||
@.planning/phases/06-sql-consolidation-code-quality/06-RESEARCH.md
|
||||
@.planning/phases/06-sql-consolidation-code-quality/06-01-SUMMARY.md
|
||||
|
||||
@backend/queue/persistence.go
|
||||
@backend/database/search.go
|
||||
@backend/library/library.go
|
||||
@backend/library/rescan.go
|
||||
@backend/database/sql/queries/audio_files.sql
|
||||
@backend/database/sqlc.yaml
|
||||
|
||||
<interfaces>
|
||||
<!-- Key types the executor needs -->
|
||||
|
||||
From backend/queue/persistence.go:
|
||||
```go
|
||||
type trackMeta struct {
|
||||
AudioFileID int64
|
||||
FilePath string
|
||||
Title string
|
||||
Artist string
|
||||
}
|
||||
|
||||
// lookupTrackMetaBatch — chunks at maxSQLiteVars (900) and calls lookupChunk per chunk
|
||||
// lookupChunk — hand-crafted SELECT with fmt.Sprintf IN clause (TARGET for sqlc migration)
|
||||
// insertTrackBatch — multi-row INSERT with variable VALUES count (STAYS hand-crafted)
|
||||
const maxSQLiteVars = 900
|
||||
```
|
||||
|
||||
From backend/database/search.go (after Plan 01 consolidation):
|
||||
- SearchFTS — FTS5 MATCH query using track_metadata VIEW
|
||||
- SearchFTSByFilename — FTS5 MATCH query using track_metadata VIEW
|
||||
- InsertSearchIndex — single-row INSERT INTO search_index
|
||||
- DeleteSearchIndex — DELETE FROM search_index WHERE rowid = ?
|
||||
- ClearSearchIndex — DELETE FROM search_index
|
||||
- RebuildSearchIndex — INSERT INTO search_index SELECT FROM track_metadata
|
||||
- SearchFTSTracks — FTS5 MATCH query using track_metadata VIEW
|
||||
|
||||
From backend/library/library.go:
|
||||
- commitNewAudioFile (~line 798) — INSERT INTO search_index VALUES (single row)
|
||||
- updateAudioFileMetadata (~line 879) — DELETE FROM search_index WHERE rowid = ?
|
||||
- updateAudioFileMetadata (~line 893) — INSERT INTO search_index VALUES (single row)
|
||||
|
||||
From backend/library/rescan.go:
|
||||
- clearAllLibraryData (~line 165) — DELETE FROM search_index
|
||||
|
||||
Complete SAFETY comment inventory (12 statements):
|
||||
| # | File | Function | Operation | Why hand-crafted |
|
||||
|---|------|----------|-----------|-----------------|
|
||||
| 1 | search.go | SearchFTS | FTS5 MATCH | FTS5 unsupported by sqlc |
|
||||
| 2 | search.go | SearchFTSByFilename | FTS5 MATCH | FTS5 unsupported by sqlc |
|
||||
| 3 | search.go | InsertSearchIndex | FTS5 INSERT | FTS5 virtual table |
|
||||
| 4 | search.go | DeleteSearchIndex | FTS5 DELETE | FTS5 virtual table |
|
||||
| 5 | search.go | ClearSearchIndex | FTS5 DELETE | FTS5 virtual table |
|
||||
| 6 | search.go | RebuildSearchIndex | FTS5 INSERT SELECT | FTS5 virtual table |
|
||||
| 7 | search.go | SearchFTSTracks | FTS5 MATCH | FTS5 unsupported by sqlc |
|
||||
| 8 | library.go | commitNewAudioFile | FTS5 INSERT | FTS5 virtual table |
|
||||
| 9 | library.go | updateAudioFileMetadata | FTS5 DELETE | FTS5 virtual table |
|
||||
| 10 | library.go | updateAudioFileMetadata | FTS5 INSERT | FTS5 virtual table |
|
||||
| 11 | rescan.go | clearAllLibraryData | FTS5 DELETE | FTS5 virtual table |
|
||||
| 12 | persistence.go | insertTrackBatch | Variable-count multi-row INSERT | sqlc can't generate variable-length batch INSERTs |
|
||||
</interfaces>
|
||||
</context>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 1: Migrate lookupChunk to sqlc with sqlc.slice()</name>
|
||||
<files>
|
||||
backend/database/sql/queries/audio_files.sql
|
||||
backend/database/sql/sqlcgen/audio_files.sql.go
|
||||
backend/database/sql/sqlcgen/models.go
|
||||
backend/queue/persistence.go
|
||||
</files>
|
||||
<action>
|
||||
1. Add the sqlc query to `backend/database/sql/queries/audio_files.sql`:
|
||||
```sql
|
||||
-- name: LookupTrackMetaByPaths :many
|
||||
SELECT id, file_path, title, artist_name
|
||||
FROM track_metadata
|
||||
WHERE file_path IN (sqlc.slice('paths'));
|
||||
```
|
||||
This uses the `track_metadata` VIEW created by Plan 01. The VIEW's columns `title` and `artist_name` match the data lookupChunk currently fetches via its inline JOIN.
|
||||
|
||||
2. Run `go tool sqlc generate` from `backend/database/` to generate the Go code.
|
||||
|
||||
3. Update `backend/queue/persistence.go`:
|
||||
|
||||
a. Replace the `lookupChunk` method body. Instead of building `fmt.Sprintf` placeholders, call the sqlc-generated `LookupTrackMetaByPaths` method:
|
||||
```go
|
||||
func (q *Queue) lookupChunk(
|
||||
paths []string,
|
||||
result map[string]trackMeta,
|
||||
) {
|
||||
if len(paths) == 0 {
|
||||
return
|
||||
}
|
||||
|
||||
rows, err := q.db.Queries.LookupTrackMetaByPaths(q.db.Ctx, paths)
|
||||
if err != nil {
|
||||
q.logger.Error("Batch metadata lookup failed", "err", err)
|
||||
return
|
||||
}
|
||||
|
||||
for _, row := range rows {
|
||||
result[row.FilePath] = trackMeta{
|
||||
AudioFileID: row.ID,
|
||||
FilePath: row.FilePath,
|
||||
Title: row.Title,
|
||||
Artist: row.ArtistName,
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
b. The `lookupTrackMetaBatch` function stays unchanged — it still chunks at `maxSQLiteVars` and calls `lookupChunk` per chunk. The chunking is still necessary because `sqlc.slice()` does NOT auto-chunk.
|
||||
|
||||
c. Remove the now-unused imports: `"fmt"` and `"strings"` may become unused if `insertTrackBatch` is the only remaining user. Check import usage — `fmt` is still needed for `insertTrackBatch` (line ~200 `fmt.Errorf`), and `strings` is still needed for `insertTrackBatch` (line ~196 `strings.Join`). Keep both if still referenced.
|
||||
|
||||
4. Verify the field name mapping is correct:
|
||||
- VIEW column `id` → sqlc field `ID` → `trackMeta.AudioFileID`
|
||||
- VIEW column `file_path` → sqlc field `FilePath` → `trackMeta.FilePath`
|
||||
- VIEW column `title` → sqlc field `Title` → `trackMeta.Title`
|
||||
- VIEW column `artist_name` → sqlc field `ArtistName` → `trackMeta.Artist`
|
||||
|
||||
5. Run queue tests to verify the migration doesn't break metadata resolution.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd backend/database && go tool sqlc generate && cd ../.. && go test -tags webkit2_41 -race -count=1 -timeout 60s ./backend/queue/...</automated>
|
||||
</verify>
|
||||
<done>
|
||||
- sqlc query `LookupTrackMetaByPaths` exists in audio_files.sql
|
||||
- lookupChunk uses the sqlc-generated query instead of fmt.Sprintf
|
||||
- lookupTrackMetaBatch still chunks at maxSQLiteVars (900)
|
||||
- All queue tests pass with -race (29 tests)
|
||||
- No hand-crafted SQL remains in lookupChunk
|
||||
</done>
|
||||
</task>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 2: Add SAFETY comments to all hand-crafted SQL</name>
|
||||
<files>
|
||||
backend/database/search.go
|
||||
backend/library/library.go
|
||||
backend/library/rescan.go
|
||||
backend/queue/persistence.go
|
||||
</files>
|
||||
<action>
|
||||
Add `// SAFETY:` comments to all 12 hand-crafted SQL statements. Each comment has two parts: (1) WHY sqlc can't handle it, and (2) what makes the query safe. Cross-reference related operations where applicable.
|
||||
|
||||
**backend/database/search.go** (7 statements):
|
||||
|
||||
1. Before SearchFTS query (~line 34):
|
||||
`// SAFETY: FTS5 MATCH syntax unsupported by sqlc. Query is parameterized; no string interpolation.`
|
||||
|
||||
2. Before SearchFTSByFilename query (~line 92):
|
||||
`// SAFETY: FTS5 MATCH syntax unsupported by sqlc. Query is parameterized; no string interpolation.`
|
||||
|
||||
3. Before InsertSearchIndex query (~line 133):
|
||||
`// SAFETY: FTS5 virtual table INSERT unsupported by sqlc. All values are parameterized.`
|
||||
|
||||
4. Before DeleteSearchIndex query (~line 143):
|
||||
`// SAFETY: FTS5 virtual table DELETE unsupported by sqlc. Rowid is parameterized.`
|
||||
|
||||
5. Before ClearSearchIndex query (~line 152):
|
||||
`// SAFETY: FTS5 virtual table DELETE unsupported by sqlc. No parameters; unconditional delete.`
|
||||
|
||||
6. Before RebuildSearchIndex query (~line 168):
|
||||
`// SAFETY: FTS5 virtual table INSERT unsupported by sqlc. All values sourced from track_metadata VIEW; no user input.`
|
||||
|
||||
7. Before SearchFTSTracks query (~line 232):
|
||||
`// SAFETY: FTS5 MATCH syntax unsupported by sqlc. Query is parameterized; no string interpolation.`
|
||||
|
||||
**backend/library/library.go** (3 statements):
|
||||
|
||||
8. Before commitNewAudioFile FTS INSERT (~line 798):
|
||||
`// SAFETY: FTS5 virtual table, see search.go:InsertSearchIndex. All values parameterized.`
|
||||
|
||||
9. Before updateAudioFileMetadata FTS DELETE (~line 879):
|
||||
`// SAFETY: FTS5 virtual table, see search.go:DeleteSearchIndex. Rowid parameterized.`
|
||||
|
||||
10. Before updateAudioFileMetadata FTS INSERT (~line 893):
|
||||
`// SAFETY: FTS5 virtual table, see search.go:InsertSearchIndex. All values parameterized.`
|
||||
|
||||
**backend/library/rescan.go** (1 statement):
|
||||
|
||||
11. Before clearAllLibraryData FTS DELETE (~line 165):
|
||||
`// SAFETY: FTS5 virtual table, see search.go:ClearSearchIndex. No parameters; unconditional delete.`
|
||||
|
||||
**backend/queue/persistence.go** (1 statement):
|
||||
|
||||
12. Before insertTrackBatch query (~line 195):
|
||||
`// SAFETY: Multi-row INSERT with variable row count unsupported by sqlc. Placeholder count matches args length; no string interpolation.`
|
||||
|
||||
**Rules:**
|
||||
- Place each SAFETY comment on the line immediately before the SQL string literal (the query variable or inline string)
|
||||
- Use the exact `// SAFETY:` prefix (capital, colon, space)
|
||||
- Two-part format: reason + safety assurance
|
||||
- Cross-reference related operations in library.go/rescan.go back to search.go
|
||||
</action>
|
||||
<verify>
|
||||
<automated>test $(grep -r "// SAFETY:" backend/database/search.go backend/library/library.go backend/library/rescan.go backend/queue/persistence.go | wc -l) -eq 12 && echo "All 12 SAFETY comments present" && go build -tags webkit2_41 ./...</automated>
|
||||
</verify>
|
||||
<done>
|
||||
- All 12 hand-crafted SQL statements have SAFETY comments
|
||||
- Comments follow two-part format (why + safety assurance)
|
||||
- Cross-references link library.go/rescan.go back to search.go
|
||||
- Code compiles without errors
|
||||
- No SAFETY comments on migration DDL (migration2, migration3, migration4)
|
||||
</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<verification>
|
||||
```bash
|
||||
# 1. sqlc generates cleanly
|
||||
cd backend/database && go tool sqlc generate
|
||||
|
||||
# 2. All queue tests pass
|
||||
go test -tags webkit2_41 -race -count=1 -timeout 60s ./backend/queue/...
|
||||
|
||||
# 3. All database tests pass
|
||||
go test -tags webkit2_41 -race -count=1 -timeout 60s ./backend/database/...
|
||||
|
||||
# 4. All library tests pass
|
||||
go test -tags webkit2_41 -race -count=1 -timeout 60s ./backend/library/...
|
||||
|
||||
# 5. Verify all 12 SAFETY comments exist
|
||||
grep -r "// SAFETY:" backend/database/search.go backend/library/library.go backend/library/rescan.go backend/queue/persistence.go | wc -l # Should be 12
|
||||
|
||||
# 6. Verify no fmt.Sprintf remains in lookupChunk
|
||||
grep -A5 "func.*lookupChunk" backend/queue/persistence.go | grep -c "fmt.Sprintf" # Should be 0
|
||||
|
||||
# 7. Full build
|
||||
go build -tags webkit2_41 ./...
|
||||
```
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
- lookupChunk uses sqlc-generated `LookupTrackMetaByPaths` query against track_metadata VIEW
|
||||
- fmt.Sprintf placeholder construction eliminated from lookupChunk
|
||||
- Chunking logic preserved (maxSQLiteVars = 900)
|
||||
- All 12 hand-crafted SQL statements documented with // SAFETY: comments
|
||||
- All existing tests pass (queue: 29, database: 15, library: 13)
|
||||
- Full project builds without errors
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
After completion, create `.planning/phases/06-sql-consolidation-code-quality/06-03-SUMMARY.md`
|
||||
</output>
|
||||
Reference in New Issue
Block a user