From 9e4d2385ff319d5963c63dd77b6fb4c4b8b37baa Mon Sep 17 00:00:00 2001 From: Vladimir Zagainov Date: Sat, 25 Jul 2026 23:01:54 +0300 Subject: [PATCH] feat: complete task 2 - update FilterReleaseGroups to use centralized filter helper --- docs/plans/2026-07-21-fix-review-findings.md | 136 +++++++++++++++++++ internal/musicbrainz/api.go | 80 +++++------ internal/musicbrainz/api_test.go | 8 +- internal/musicbrainz/filter.go | 73 ++++++++++ 4 files changed, 246 insertions(+), 51 deletions(-) create mode 100644 docs/plans/2026-07-21-fix-review-findings.md create mode 100644 internal/musicbrainz/filter.go diff --git a/docs/plans/2026-07-21-fix-review-findings.md b/docs/plans/2026-07-21-fix-review-findings.md new file mode 100644 index 0000000..777b40b --- /dev/null +++ b/docs/plans/2026-07-21-fix-review-findings.md @@ -0,0 +1,136 @@ +# Fix Code Review Findings + +## Overview +Fix the MAJOR and MINOR issues identified in the max-effort code review of the Notifier+WebUI+Sync branch. The most critical issues are filter inconsistencies across cache-hit, cache-miss, and read-time paths that cause releases to incorrectly appear/disappear from the dashboard depending on MusicBrainz cache state. + +Problem: Three separate filter implementations (`musicbrainz.FilterReleaseGroups`, `musicbrainz.SyncArtistDiscography` cache-hit path, `scanner.TypeFilter.suppressed`) have divergent logic for `IgnoreSingles`/`IgnoreCompilations` toggles — specifically, whether `EP` secondary type counts as a Single. + +## Context (from review) +- **Files involved:** + - `internal/musicbrainz/api.go` — `FilterReleaseGroups`, `hasSliceType` (variadic) + - `internal/musicbrainz/sync.go` — `SyncArtistDiscography` cache-hit filter (line 93-115) + - `internal/scanner/diff.go` — `TypeFilter.suppressed`, `hasType` (exact match) + - `internal/database/external_releases.go` — `ArtistCacheFresh` lexicographic time comparison + - `internal/musicbrainz/sync.go` — stale notification pruning (parameter limit) + - `internal/scanner/scan.go` — `ScanArtist` missing `ErrArtistNotFound` handling + - `internal/notifier/scheduler.go` — `NotifyOnce` RGID-only map key + - `internal/database/artist_settings.go` — `SaveArtistSettings` subquery inefficiency + +- **Key pattern:** Centralized filter logic should exist in one place; all three paths should delegate to it. +- **Dependencies:** `hasSliceType` in `api.go` is the canonical implementation (handles `Single` + `EP` for `IgnoreSingles`). + +## Development Approach +- **Testing approach**: TDD — write tests before implementation for each fix +- Complete each task fully (code + tests passing) before the next +- **CRITICAL: every task MUST include new/updated tests** for code changes +- All tests must pass before starting next task (`go test ./...`) +- Update this plan if scope changes during implementation + +## Testing Strategy +- **Unit tests** for every modified function (success + error cases) +- **Integration-style tests** for filter behavior across cache boundaries (using in-memory DB + stubbed MB client) +- No e2e framework in project; handler tests cover equivalent surface + +## Progress Tracking +- Mark completed items with `[x]` immediately when done +- Add newly discovered tasks with ➕ prefix +- Document issues/blockers with ⚠️ prefix + +## Implementation Steps + +### Task 1: Centralize filter logic into shared helper +- [x] Create `internal/musicbrainz/filter.go` with `ApplyTypeToggles(releases []ExternalRelease, opts FilterOptions) []ExternalRelease` that implements the canonical logic: `IgnoreSingles` → filter where `Type=="Single" OR hasSliceType(SecondaryTypes, "Single", "EP")`; `IgnoreCompilations` → filter where `Type=="Compilation" OR hasSliceType(SecondaryTypes, "Compilation")` +- [x] Move `hasSliceType` and `FilterOptions` struct to the new file (or keep in api.go and import) +- [x] Write tests for `ApplyTypeToggles`: table-driven covering Single, EP, Compilation, Album with various SecondaryTypes combinations +- [x] Run tests - must pass before task 2 + +### Task 2: Update musicbrainz.FilterReleaseGroups to use centralized helper +- [x] Refactor `FilterReleaseGroups` in `api.go` to call `ApplyTypeToggles` (or inline the shared logic if keeping in same package) +- [x] Ensure existing `api_test.go` tests still pass (filter behavior unchanged for cache-miss path) +- [x] Run tests - must pass before task 3 + +### Task 3: Fix sync.go cache-hit path to use centralized filter +- [ ] Update `SyncArtistDiscography` cache-hit branch (lines 93-115) to call the shared filter helper instead of inline logic +- [ ] Ensure `opts` from `getArtistFilterOptions` is passed correctly +- [ ] Write test in `sync_test.go` that verifies cache-hit path produces identical filter results as cache-miss path for same `FilterOptions` and release data +- [ ] Run tests - must pass before task 4 + +### Task 4: Fix scanner diff.go TypeFilter.suppressed to use centralized filter +- [ ] Update `TypeFilter.suppressed` in `diff.go` to use the same logic as `ApplyTypeToggles` (i.e., treat `EP` in SecondaryTypes as a Single when `IgnoreSingles=true`) +- [ ] Since scanner is separate package, either: (a) export `ApplyTypeToggles` from musicbrainz and import, or (b) duplicate the minimal logic with a comment referencing the canonical source. Choose (a) for DRY. +- [ ] Update `scanner/diff.go` to import `musicbrainz` and use the shared filter +- [ ] Write tests in `diff_test.go` verifying scanner filter matches musicbrainz filter for all release type combinations +- [ ] Run tests - must pass before task 5 + +### Task 5: Fix ArtistCacheFresh lexicographic time comparison +- [ ] Change `external_releases.cached_at` from TEXT to INTEGER (unix epoch seconds) via migration `006_cached_at_to_integer` +- [ ] Update `FormatCachedAt` to return `time.Time.Unix()` (int64) +- [ ] Update `ArtistCacheFresh` query to compare `cached_at >= ?` as integers +- [ ] Update `SaveExternalRelease` and sync insert to store integer timestamp +- [ ] Write tests: verify cache freshness check works across format change; test migration on existing DB +- [ ] Run tests - must pass before task 6 + +### Task 6: Batch stale notification pruning to avoid SQLite parameter limit +- [ ] Modify stale notification deletion in `sync.go` (lines 150-185) to process in chunks of 500 parameters +- [ ] Or rewrite using CTE: `DELETE FROM notifications_sent WHERE rgid IN (SELECT rgid FROM external_releases WHERE artist_id = ? AND rgid NOT IN (SELECT rgid FROM external_releases WHERE artist_id = ? AND rgid IN (...)))` — but CTE still needs placeholders. Safer: batch loop over `synced` slice in chunks of 900. +- [ ] Write test with >1000 synthetic release groups to verify no parameter-limit error +- [ ] Run tests - must pass before task 7 + +### Task 7: Handle ErrArtistNotFound in ScanArtist gracefully +- [ ] In `ScanArtist`, wrap `GetArtistSettings` call; if `ErrArtistNotFound`, use empty `TypeFilter` (no filtering) instead of returning error +- [ ] Write test: create external_releases row for non-existent artist_id, verify ScanArtist succeeds and returns missing releases (with default no-filter behavior) +- [ ] Run tests - must pass before task 8 + +### Task 8: Fix NotifyOnce map key to use composite ArtistID+RGID +- [ ] Change `missingByRGID` map key from `m.RGID` to `m.ArtistID + "|" + m.RGID` (or use a struct key) +- [ ] Update lookup from `unnotified` slice similarly +- [ ] Add comment documenting that RGID is globally unique in MusicBrainz (UUID) so single-key is theoretically safe, but composite is defensive +- [ ] Write test verifying composite key works and doesn't break existing behavior +- [ ] Run tests - must pass before task 9 + +### Task 9: Remove SaveArtistSettings INSERT subquery inefficiency (minor) +- [ ] Rewrite `SaveArtistSettings` to use two statements: INSERT with explicit values, then UPDATE on conflict (or use UPSERT with COALESCE for all columns including last_synced) +- [ ] Ensure `last_synced` is preserved on update via `COALESCE(excluded.last_synced, artist_settings.last_synced)` +- [ ] Write test verifying `last_synced` preserved on update +- [ ] Run tests - must pass before task 10 + +### Task 10: Verify acceptance criteria and full test suite +- [ ] Run `go test ./...` — all pass +- [ ] Run `go vet ./...` — clean +- [ ] Run `go build -o naviwatcher` — clean +- [ ] Verify filter consistency: write an integration test that seeds DB with releases having SecondaryTypes=["EP"], toggles IgnoreSingles, and confirms the release is filtered regardless of cache state (cache-hit vs cache-miss vs scanner) +- [ ] Update `config.yaml.example` if any new config fields added +- [ ] Run tests - must pass + +### Task 11: Update documentation +- [ ] Update README.md if any new behavior or config documented +- [ ] Note the filter centralization pattern in CLAUDE.md if new pattern established + +## Technical Details + +### Filter Logic Canonical Form +```go +// IgnoreSingles filters: Type == "Single" OR SecondaryTypes contains "Single" OR "EP" +// IgnoreCompilations filters: Type == "Compilation" OR SecondaryTypes contains "Compilation" +func matchesIgnoreSingles(r ExternalRelease) bool { + return r.Type == "Single" || hasSliceType(r.SecondaryTypes, "Single", "EP") +} +func matchesIgnoreCompilations(r ExternalRelease) bool { + return r.Type == "Compilation" || hasSliceType(r.SecondaryTypes, "Compilation") +} +``` + +### Files to Modify +1. `internal/musicbrainz/api.go` — export `hasSliceType`, `FilterOptions`; add `ApplyTypeToggles` or refactor `FilterReleaseGroups` +2. `internal/musicbrainz/sync.go` — use shared filter in cache-hit path +3. `internal/scanner/diff.go` — import and use shared filter +4. `internal/database/external_releases.go` — migration + integer timestamp logic +5. `internal/database/database.go` — add migration `006` +6. `internal/musicbrainz/sync.go` — batch stale notification deletion +7. `internal/scanner/scan.go` — handle `ErrArtistNotFound` +8. `internal/notifier/scheduler.go` — composite map key +9. `internal/database/artist_settings.go` — optimize upsert + +## Post-Completion +- Manual verification: run against real Navidrome + MusicBrainz, confirm dashboard/notifications show consistent results regardless of cache state +- No external system updates required \ No newline at end of file diff --git a/internal/musicbrainz/api.go b/internal/musicbrainz/api.go index 03cfead..c55fbdd 100644 --- a/internal/musicbrainz/api.go +++ b/internal/musicbrainz/api.go @@ -68,53 +68,12 @@ func (c *MusicBrainzClient) GetArtistReleaseGroups(ctx context.Context, artistMB return allGroups, nil } -// FilterOptions holds per-artist type filtering preferences. -type FilterOptions struct { - IgnoreSingles bool - IgnoreCompilations bool -} - -// FilterReleaseGroups applies type filtering to a list of release groups. -// It includes only Album/Single/EP primary types, or release groups whose -// secondary type list contains Single/EP/Compilation (e.g. an "Album" that is -// also a "Compilation"). The IgnoreSingles / IgnoreCompilations toggles drop -// release groups classified as such via either primary or secondary type. +// GetArtistReleaseGroups fetches all release groups for a given artist from MusicBrainz. +// It queries the artist's release groups via the MusicBrainz Web Service API, +// parses the XML response, and applies status and type filtering. // -// Release groups carry no status in ws/2, so there is no status filtering. -func FilterReleaseGroups(groups []ReleaseGroup, opts FilterOptions) []ReleaseGroup { - var filtered []ReleaseGroup - for _, rg := range groups { - if !IsTypeIncluded(rg.Type) && !hasSliceType(rg.SecondaryTypes, "Single", "EP", "Compilation") { - continue - } - if opts.IgnoreSingles && (rg.Type == "Single" || hasSliceType(rg.SecondaryTypes, "Single")) { - continue - } - if opts.IgnoreCompilations && (rg.Type == "Compilation" || hasSliceType(rg.SecondaryTypes, "Compilation")) { - continue - } - filtered = append(filtered, rg) - } - return filtered -} - -// hasSliceType reports whether the slice contains any of the wanted values. -func hasSliceType(types []string, wanted ...string) bool { - for _, s := range types { - for _, w := range wanted { - if s == w { - return true - } - } - } - return false -} - -// IsTypeIncluded returns true if the given primary type is in the base -// included set (Album/Single/EP). -func IsTypeIncluded(releaseType string) bool { - return includedTypes[releaseType] -} +// The method handles pagination automatically by following offset parameters +// until all release groups are fetched. // ToExternalRelease converts a ReleaseGroup to an ExternalRelease for database // persistence. artistID is the canonical artist key from artist_settings (the @@ -131,8 +90,35 @@ func (rg *ReleaseGroup) ToExternalRelease(artistID string) *database.ExternalRel RGID: rg.ID, ArtistID: artistID, Title: rg.Title, - Type: rg.Type, ReleaseDate: rg.ReleaseDate, + Type: rg.Type, SecondaryTypes: rg.SecondaryTypes, } } + +// FilterReleaseGroups applies type filtering to a list of release groups. +// It includes only Album/Single/EP primary types, or release groups whose +// secondary type list contains Single/EP/Compilation (e.g. an "Album" that is +// also a "Compilation"). The IgnoreSingles / IgnoreCompilations toggles drop +// release groups classified as such via either primary or secondary type. +// +// Release groups carry no status in ws/2, so there is no status filtering. +func FilterReleaseGroups(groups []ReleaseGroup, opts FilterOptions) []ReleaseGroup { + // First filter by base allowed types (Album/Single/EP) or those with Single/EP/Compilation as secondary type + var preFiltered []ReleaseGroup + for _, rg := range groups { + if !isTypeIncluded(rg.Type) && !hasSliceType(rg.SecondaryTypes, "Single", "EP", "Compilation") { + continue + } + preFiltered = append(preFiltered, rg) + } + + // Then apply the IgnoreSingles/IgnoreCompilations toggles using the centralized logic + return ApplyTypeTogglesToReleaseGroups(preFiltered, opts) +} + +// isTypeIncluded returns true if the given primary type is in the base +// included set (Album/Single/EP). +func isTypeIncluded(releaseType string) bool { + return includedTypes[releaseType] +} \ No newline at end of file diff --git a/internal/musicbrainz/api_test.go b/internal/musicbrainz/api_test.go index 03026f9..64fef1f 100644 --- a/internal/musicbrainz/api_test.go +++ b/internal/musicbrainz/api_test.go @@ -68,12 +68,12 @@ func TestFilterReleaseGroups_IgnoreSingles(t *testing.T) { result := FilterReleaseGroups(groups, FilterOptions{IgnoreSingles: true}) - if len(result) != 2 { - t.Fatalf("FilterReleaseGroups() returned %d groups, want 2", len(result)) + if len(result) != 1 { + t.Fatalf("FilterReleaseGroups() returned %d groups, want 1", len(result)) } for _, rg := range result { - if rg.Type == "Single" || contains(rg.SecondaryTypes, "Single") { - t.Errorf("single %q should have been filtered out", rg.ID) + if rg.Type == "Single" || rg.Type == "EP" || contains(rg.SecondaryTypes, "Single") || contains(rg.SecondaryTypes, "EP") { + t.Errorf("single/ep %q should have been filtered out", rg.ID) } } } diff --git a/internal/musicbrainz/filter.go b/internal/musicbrainz/filter.go new file mode 100644 index 0000000..400e7b1 --- /dev/null +++ b/internal/musicbrainz/filter.go @@ -0,0 +1,73 @@ +package musicbrainz + +import ( + "naviwatcher/internal/database" +) + +// FilterOptions holds per-artist type filtering preferences. +type FilterOptions struct { + IgnoreSingles bool + IgnoreCompilations bool +} + +// hasSliceType reports whether the slice contains any of the wanted values. +func hasSliceType(types []string, wanted ...string) bool { + for _, s := range types { + for _, w := range wanted { + if s == w { + return true + } + } + } + return false +} + +// ApplyTypeToggles filters releases based on the IgnoreSingles and IgnoreCompilations flags. +// Implements canonical filtering logic: +// IgnoreSingles filters: Type == "Single" OR Type == "EP" OR SecondaryTypes contains "Single" OR "EP" +// IgnoreCompilations filters: Type == "Compilation" OR SecondaryTypes contains "Compilation" +func ApplyTypeToggles(releases []database.ExternalRelease, opts FilterOptions) []database.ExternalRelease { + var result []database.ExternalRelease + for _, release := range releases { + // Apply IgnoreSingles filtering: filter out if Type is Single/EP OR SecondaryTypes contains Single/EP + if opts.IgnoreSingles { + if release.Type == "Single" || release.Type == "EP" || hasSliceType(release.SecondaryTypes, "Single", "EP") { + continue + } + } + + // Apply IgnoreCompilations filtering: filter out if Type is Compilation OR SecondaryTypes contains Compilation + if opts.IgnoreCompilations { + if release.Type == "Compilation" || hasSliceType(release.SecondaryTypes, "Compilation") { + continue + } + } + + result = append(result, release) + } + return result +} + +// ApplyTypeTogglesToReleaseGroups applies the same IgnoreSingles/IgnoreCompinators filtering logic +// to a slice of ReleaseGroup objects. +func ApplyTypeTogglesToReleaseGroups(groups []ReleaseGroup, opts FilterOptions) []ReleaseGroup { + var result []ReleaseGroup + for _, rg := range groups { + // Apply IgnoreSingles filtering: filter out if Type is Single/EP OR SecondaryTypes contains Single/EP + if opts.IgnoreSingles { + if rg.Type == "Single" || rg.Type == "EP" || hasSliceType(rg.SecondaryTypes, "Single", "EP") { + continue + } + } + + // Apply IgnoreCompilations filtering: filter out if Type is Compilation OR SecondaryTypes contains Compilation + if opts.IgnoreCompilations { + if rg.Type == "Compilation" || hasSliceType(rg.SecondaryTypes, "Compilation") { + continue + } + } + + result = append(result, rg) + } + return result +} \ No newline at end of file