NaviWatcher/docs/plans/2026-07-21-fix-review-findings.md

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

• 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")
• Move hasSliceType and FilterOptions struct to the new file (or keep in api.go and import)
• Write tests for ApplyTypeToggles: table-driven covering Single, EP, Compilation, Album with various SecondaryTypes combinations
• Run tests - must pass before task 2

Task 2: Update musicbrainz.FilterReleaseGroups to use centralized helper

• Refactor FilterReleaseGroups in api.go to call ApplyTypeToggles (or inline the shared logic if keeping in same package)
• Ensure existing api_test.go tests still pass (filter behavior unchanged for cache-miss path)
• Run tests - must pass before task 3

Task 3: Fix sync.go cache-hit path to use centralized filter

• Update SyncArtistDiscography cache-hit branch 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 010_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
• 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