191 lines
11 KiB
Markdown
191 lines
11 KiB
Markdown
# Notifier, Web UI & Sync Wiring
|
||
|
||
## Overview
|
||
Transform NaviWatcher from a compute-only daemon (which only logs missing
|
||
releases) into a working service:
|
||
1. **Sync wiring** in `main.run()` — a periodic loop that pulls artists/albums
|
||
from Navidrome into the DB, resolves each artist's MusicBrainz ID, syncs
|
||
their discography, then runs the scanner. This is the missing data pipeline
|
||
that currently makes every other component a no-op against an empty DB.
|
||
2. **Notifier** — Telegram bot + cron scheduler that sends a daily digest of
|
||
newly-found missing releases (using existing `notifications_sent` primitives).
|
||
3. **Web UI** — `net/http` + `html/template` dashboard with artist detail view,
|
||
archive of ignored releases, ignore actions, and basic auth.
|
||
|
||
Problem solved: today `main.run()` calls `scanner.ScanAll` over empty tables and
|
||
blocks on `<-ctx.Done()`. Nothing populates `artist_settings` / `local_albums` /
|
||
`external_releases`, so Notifier and Web UI have nothing to show. This plan
|
||
closes that gap end-to-end.
|
||
|
||
Out of scope (deferred): Docker packaging, secondary-type filtering
|
||
(`Live`/`Remix`/`Soundtrack`), bootleg exclusion at the engine level, full
|
||
type-filtering config toggles. Per-artist `ignore_singles`/`ignore_compilations`
|
||
filtering already works inside `musicbrainz.SyncArtistDiscography`.
|
||
|
||
## Context (from discovery)
|
||
- `internal/navidrome/{client,sync}.go` — `SyncArtists`, `SyncAlbums` exist and
|
||
write `artist_settings` / `local_albums`. **Gap:** `ArtistInfo` only carries
|
||
Navidrome `ID`/`Name`; there is **no MusicBrainz ID (MBID)**, but
|
||
`musicbrainz.SyncArtistDiscography` requires `artistMBID`.
|
||
- `internal/musicbrainz/{client,sync,api}.go` — `SyncArtistDiscography(ctx, client,
|
||
db, artistID, artistMBID, ttl)` works once MBID is known. `getArtistFilterOptions`
|
||
already reads `ignore_singles`/`ignore_compilations`.
|
||
- `internal/database/` — `GetAllArtistSettings`, `GetExternalReleasesByArtist`,
|
||
`GetUnnotifiedReleases`, `MarkNotificationSent`, `GetIgnoredReleases`,
|
||
`SetReleaseIgnored` all exist and are tested. `artist_settings` schema has no
|
||
MBID column.
|
||
- `internal/scanner/scan.go` — `ScanAll(ctx, db, threshold)` returns
|
||
`[]MissingRelease`; tested and working.
|
||
- `cmd/naviwatcher/main.go` — `App` holds cfg/db/mbClient only; `run()` is the
|
||
compute-only stub. `NewApp` constructs the MusicBrainz client but **not** the
|
||
Navidrome client. No goroutines for sync/notifier/web.
|
||
- `internal/config/config.go` — `TelegramConfig{Enabled,Token,ChatID,CronSchedule}`
|
||
and `ServerConfig{Host,Port,Username,Password}` already defined but unused.
|
||
- `config.yaml.example` exists.
|
||
|
||
### Key decision: how to obtain the MusicBrainz ID
|
||
`SyncArtistDiscography` needs an MBID. Navidrome's Subsonic API does not return
|
||
MBIDs via `getArtists`/`getArtist`. Resolution: **add an `mbid` column to
|
||
`artist_settings`** and resolve it lazily during sync by querying MusicBrainz
|
||
artist search (`/ws/2/artist/?query=artist:<name>&fmt=json`). Cache the MBID on
|
||
the artist row. This avoids manual config and keeps the schema the single source
|
||
of truth. (Alternative considered: resolve by name on every sync without
|
||
storing — rejected because it doubles rate-limited MB calls and is flaky on
|
||
name collisions.)
|
||
|
||
## Development Approach
|
||
- **Testing approach**: TDD — write tests before implementation for each task.
|
||
- Complete each task fully (code + tests passing) before the next.
|
||
- Every task MUST include new/updated tests (success + error/edge cases).
|
||
- All tests must pass before starting the next task.
|
||
- Run `go test ./...` and `go vet ./...` after each task.
|
||
- Maintain backward compatibility of existing DB schema (additive migration only).
|
||
|
||
## Testing Strategy
|
||
- **Unit tests** for every new function/method (success + error paths).
|
||
- **Integration-style tests** for sync/resolver using a `:memory:` DB and a
|
||
stubbed MusicBrainz HTTP client (the existing `client_test.go` already shows
|
||
the httptest pattern — reuse it).
|
||
- **Web UI**: table-driven tests for handlers (status codes, auth rejection,
|
||
ignore action effects on DB) using `httptest.NewServer` + in-memory DB. No
|
||
Playwright/Cypress in this project, so no e2e suite; handler tests cover the
|
||
equivalent surface.
|
||
- **Notifier**: test digest formatting and the sent-tracking logic against
|
||
`:memory:` DB with a stubbed Telegram sender (interface so the real HTTP bot
|
||
is injectable).
|
||
|
||
## Progress Tracking
|
||
- Mark completed items with `[x]` immediately when done.
|
||
- Add newly discovered tasks with ➕ prefix.
|
||
- Document issues/blockers with ⚠️ prefix.
|
||
- Keep plan in sync with actual work.
|
||
|
||
## Implementation Steps
|
||
|
||
### Task 1: Add `mbid` column to artist_settings
|
||
- [x] add migration `006_add_mbid_to_artist_settings` (`ALTER TABLE artist_settings ADD COLUMN mbid TEXT;`)
|
||
- [x] extend `ArtistSettings` struct + `SaveArtistSettings`/`UpsertArtist` to persist `MBID`
|
||
- [x] write tests for migration + struct round-trip (empty MBID allowed, set/get)
|
||
- [x] run tests - must pass before task 2
|
||
|
||
### Task 2: MusicBrainz artist-ID resolver
|
||
- [x] add `ResolveArtistMBID(ctx, client, name) (string, error)` in `internal/musicbrainz` using `/ws/2/artist/?query=artist:<name>&fmt=json`
|
||
- [x] parse first matching artist ID from JSON response; return error if none
|
||
- [x] write tests with httptest stub (match found, no match, HTTP error)
|
||
- [x] run tests - must pass before task 3
|
||
|
||
### Task 3: Periodic sync pipeline
|
||
- [x] add `SyncAll(ctx, ndClient, mbClient, db, ttl)` orchestrator: for each monitored artist → ensure MBID (resolve + persist if missing) → `musicbrainz.SyncArtistDiscography` → `navidrome.SyncAlbums`
|
||
- [x] wire `navidrome.NewClient` into `App`; add `ndClient` field
|
||
- [x] write tests for `SyncAll` with stubbed clients + `:memory:` DB (new artist gets MBID, existing MBID reused, unmonitored skipped)
|
||
- [x] run tests - must pass before task 4
|
||
|
||
### Task 4: Main loop wiring (sync → scan)
|
||
- [x] replace compute-only `run()` with: one immediate sync+scan, then a ticker-driven periodic sync+scan goroutine; keep graceful shutdown via ctx
|
||
- [x] add a `syncInterval` config field (default e.g. 6h) to `config.go` + defaults + validation
|
||
- [x] write tests for the loop scheduling logic where feasible (ticker fires, ctx cancels cleanly)
|
||
- [x] run tests - must pass before task 5
|
||
|
||
### Task 5: Notifier — Telegram sender + digest
|
||
- [x] define `Sender` interface (`Send(ctx, message string) error`) and a `telegramSender` using `TelegramConfig` (bot API `sendMessage`)
|
||
- [x] add `FormatDigest(missing []scanner.MissingRelease, uiBaseURL string) string` (artist names + counts + Web UI link)
|
||
- [x] write tests: digest formatting, sender failure handling (stub sender)
|
||
- [x] run tests - must pass before task 6
|
||
|
||
### Task 6: Notifier — scheduler + sent-tracking
|
||
- [x] add `NotifyOnce(ctx, db, sender, cfg)` : query `GetUnnotifiedReleases`, build digest, send, `MarkNotificationSent` per rgid
|
||
- [x] add cron-based scheduler goroutine honoring `TelegramConfig.CronSchedule` (use a lightweight cron lib or robfig/cron); no-op if `Enabled=false`
|
||
- [x] write tests: `NotifyOnce` marks sent and skips already-sent; scheduler parses cron and fires (inject fixed time / use every-minute for test)
|
||
- [x] run tests - must pass before task 7
|
||
|
||
### Task 7: Web UI — server + auth + dashboard
|
||
- [x] create `internal/web` with `Server` (net/http), `//go:embed` templates, basic-auth middleware using `ServerConfig.Username/Password`
|
||
- [x] dashboard handler: list monitored artists with missing-release counts (join scanner result / external vs local)
|
||
- [x] write tests: unauthenticated request → 401; authenticated → 200 with expected artist rendered
|
||
- [x] run tests - must pass before task 8
|
||
|
||
### Task 8: Web UI — artist detail + archive + ignore actions
|
||
- [x] artist page: local albums (Subsonic) + found missing (MB cache) + ignore buttons
|
||
- [x] archive page: `GetIgnoredReleases` with restore action
|
||
- [x] POST handlers: `SetReleaseIgnored(rgid, true/false)`; "ignore all singles of artist" toggles `artist_settings.ignore_singles`
|
||
- [x] write tests: ignore sets flag + removes from dashboard missing; restore clears flag; auth enforced on POST
|
||
- [x] run tests - must pass before task 9
|
||
|
||
### Task 9: Verify acceptance criteria
|
||
- [x] run full suite `go test ./...` — all pass
|
||
- [x] run `go vet ./...` and `go build -o naviwatcher` — clean
|
||
- [x] verify scan→notify→web data flow with a seeded `:memory:`/file DB smoke check
|
||
- [x] verify config.yaml.example documents new `sync_interval` field
|
||
|
||
### Task 10: Update documentation
|
||
- [x] add a short "How it works now" note to README/CLAUDE.md if present
|
||
- [x] note the new `sync_interval` config key in `config.yaml.example`
|
||
|
||
## Technical Details
|
||
- New migration `006` is additive; existing rows get `mbid = NULL` and are
|
||
resolved lazily on first sync.
|
||
- `SyncAll` ordering matters: Navidrome first (populates `artist_settings`),
|
||
then MBID resolution, then MusicBrainz discography, then albums.
|
||
- Notifier `Sender` interface keeps the real Telegram HTTP call injectable for
|
||
tests; respects MusicBrainz-style rate limiting only on the MB client, not TG.
|
||
- Web UI basic auth uses `crypto/subtle.ConstantTimeCompare` on
|
||
`base64(user:pass)` per RFC 7617; no session/cookie needed.
|
||
- Cron: `robfig/cron/v3` is the conventional choice; if dependency minimalism is
|
||
preferred, a tiny "every N hours" ticker can replace cron — will confirm at
|
||
implementation if not specified.
|
||
|
||
## Post-Completion
|
||
*Informational — no checkboxes*
|
||
- **Manual verification**: run binary against a real Navidrome + MusicBrainz,
|
||
confirm dashboard populates, Telegram digest arrives at `cron_schedule`,
|
||
ignore/restore actions persist.
|
||
- **External**: ensure `config.yaml.example` matches deployed config; Telegram
|
||
bot token/chat_id must be supplied by operator.
|
||
|
||
## Follow-up fixes (post code review)
|
||
After the plan's Tasks 1-10 merged, a code review surfaced and fixed:
|
||
- **Duplicate notifications**: `SyncArtistDiscography` previously deleted
|
||
`notifications_sent` for the whole artist on every cache-miss re-sync, which
|
||
wiped "already notified" tracking and re-sent digests. Now only notifications
|
||
for releases that disappear are pruned, and surviving releases keep their sent
|
||
markers. Re-sync is FK-safe (uses `INSERT OR REPLACE` + `rgid NOT IN (…)`).
|
||
- **Empty-discography caching**: artists with zero MusicBrainz release groups
|
||
were never cached (a `0`-row result was treated as a cache miss), re-fetching
|
||
every cycle. Added `artist_settings.last_synced` (migration `009`) as the cache
|
||
freshness signal so empty discographies honor the TTL.
|
||
- **`main.run` wiring**: the Web UI server and Telegram notifier scheduler are
|
||
now constructed in `NewApp` and started as goroutines in `run()` (previously
|
||
only the sync loop ran).
|
||
- **Overlap guard**: `startPeriodicSync` now skips a tick while a previous sync
|
||
is still in flight (buffered `done` channel) so syncs never overlap.
|
||
- **`server.public_url` config**: added so Telegram digest links use an
|
||
externally-reachable origin instead of the bind `host:port` (which defaults to
|
||
`0.0.0.0`). `NewServerWithConfig` falls back to host:port only for a real host.
|
||
- **Web handlers**: artist detail page now uses `scanner.ScanArtist` (per-artist)
|
||
instead of a full `ScanAll`; removed the always-false `releaseIgnored` lookup
|
||
and dead `endsWith` helper; the configured fuzzy threshold is now threaded
|
||
through `Server`.
|
||
- **DB connection pooling**: `:memory:` databases now use `SetMaxOpenConns(1)`
|
||
so migrations and queries share one in-memory store (prevents "missing column"
|
||
errors under the connection pool).
|