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

@@ -56,11 +56,11 @@ Problem: Three separate filter implementations (`musicbrainz.FilterReleaseGroups
 - [x] 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
+- [x] 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`)
+- [x] 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.
+- [x] Update `scanner/diff.go` to import `musicbrainz` and use the shared filter
+- [x] Write tests in `diff_test.go` verifying scanner filter matches musicbrainz filter for all release type combinations
+- [x] 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`

internal/scanner/diff.go

@@ -2,6 +2,7 @@ package scanner
 
 import (
 	"naviwatcher/internal/database"
+	"naviwatcher/internal/musicbrainz"
 )
 
 // MissingRelease describes an external release that has no sufficiently similar
@@ -34,23 +35,13 @@ type TypeFilter struct {
 // secondary types, matching musicbrainz.FilterReleaseGroups so both the
 // cache-miss (store-time) and read-time paths agree.
 func (f TypeFilter) suppressed(ext database.ExternalRelease) bool {
-	if f.IgnoreSingles && (ext.Type == "Single" || hasType(ext.SecondaryTypes, "Single")) {
-		return true
+	// Use the centralized filtering logic from musicbrainz package
+	opts := musicbrainz.FilterOptions{
+		IgnoreSingles:      f.IgnoreSingles,
+		IgnoreCompilations: f.IgnoreCompilations,
 	}
-	if f.IgnoreCompilations && (ext.Type == "Compilation" || hasType(ext.SecondaryTypes, "Compilation")) {
-		return true
-	}
-	return false
-}
-
-// hasType reports whether types contains want.
-func hasType(types []string, want string) bool {
-	for _, t := range types {
-		if t == want {
-			return true
-		}
-	}
-	return false
+	filtered := musicbrainz.ApplyTypeToggles([]database.ExternalRelease{ext}, opts)
+	return len(filtered) == 0
 }
 
 // FindMissingReleases compares an artist's external discography against the

internal/scanner/diff_test.go

@@ -0,0 +1,150 @@
+package scanner
+
+import (
+	"testing"
+
+	"naviwatcher/internal/database"
+	"naviwatcher/internal/musicbrainz"
+)
+
+func TestTypeFilterSuppressedMatchesMusicbrainzFilter(t *testing.T) {
+	// Test cases covering various combinations of types and secondary types
+	testCases := []struct {
+		name               string
+		releaseType        string
+		secondaryTypes     []string
+		ignoreSingles      bool
+		ignoreCompilations bool
+		expectedSuppressed bool
+	}{
+		// Single type tests
+		{"Single primary type", "Single", []string{}, true, false, true},
+		{"Single primary type with EP ignore", "Single", []string{}, false, true, false},
+
+		// EP as primary type (should be treated as Single when IgnoreSingles=true)
+		{"EP primary type", "EP", []string{}, true, false, true},
+		{"EP primary type with EP ignore", "EP", []string{}, false, true, false},
+
+		// Album type tests
+		{"Album primary type", "Album", []string{}, true, false, false},
+		{"Album primary type with Compilation ignore", "Album", []string{}, false, true, false},
+
+		// Compilation type tests
+		{"Compilation primary type", "Compilation", []string{}, true, false, false},
+		{"Compilation primary type with Compilation ignore", "Compilation", []string{}, false, true, true},
+
+		// Secondary types - Single
+		{"Album with Single secondary", "Album", []string{"Single"}, true, false, true},
+		{"Album with Single secondary (no ignore)", "Album", []string{"Single"}, false, false, false},
+		{"EP with Single secondary", "EP", []string{"Single"}, true, false, true},
+
+		// Secondary types - EP (should trigger Single ignore)
+		{"Album with EP secondary", "Album", []string{"EP"}, true, false, true},
+		{"Album with EP secondary (no ignore)", "Album", []string{"EP"}, false, false, false},
+
+		// Secondary types - Compilation
+		{"Album with Compilation secondary", "Album", []string{"Compilation"}, true, false, false},
+		{"Album with Compilation secondary (with ignore)", "Album", []string{"Compilation"}, false, true, true},
+
+		// Multiple secondary types
+		{"Album with Single and EP secondary", "Album", []string{"Single", "EP"}, true, false, true},
+		{"Album with Compilation secondary", "Album", []string{"Compilation"}, false, true, true},
+		{"Album with multiple secondary types", "Album", []string{"Single", "Compilation"}, true, true, true},
+
+		// Edge cases
+		{"Empty types", "", []string{}, false, false, false},
+		{"Unknown type", "Live", []string{}, false, false, false},
+	}
+
+	for _, tc := range testCases {
+		tc := tc // capture range variable
+		t.Run(tc.name, func(t *testing.T) {
+			// Create test release
+			release := database.ExternalRelease{
+				Type:           tc.releaseType,
+				SecondaryTypes: tc.secondaryTypes,
+			}
+
+			// Test scanner filter
+			scannerFilter := TypeFilter{
+				IgnoreSingles:      tc.ignoreSingles,
+				IgnoreCompilations: tc.ignoreCompilations,
+			}
+			scannerSuppressed := scannerFilter.suppressed(release)
+
+			// Test musicbrainz filter
+			mbFilter := musicbrainz.FilterOptions{
+				IgnoreSingles:      tc.ignoreSingles,
+				IgnoreCompilations: tc.ignoreCompilations,
+			}
+			mbFiltered := musicbrainz.ApplyTypeToggles([]database.ExternalRelease{release}, mbFilter)
+			mbSuppressed := len(mbFiltered) == 0
+
+			// Both should agree
+			if scannerSuppressed != mbSuppressed {
+				t.Errorf("Scanner and MusicBrainz filter disagree for %v: scanner=%v, musicbrainz=%v",
+					tc, scannerSuppressed, mbSuppressed)
+			}
+
+            // Check against expected value
+			if scannerSuppressed != tc.expectedSuppressed {
+				t.Errorf("Scanner filter returned %v, expected %v for case %v",
+					scannerSuppressed, tc.expectedSuppressed, tc.name)
+			}
+		})
+	}
+}
+
+// Test that verifies the specific case mentioned in the issue: EP in SecondaryTypes counts as Single
+func TestTypeFilterTreatsEPAsSingleWhenIgnoreSingles(t *testing.T) {
+	testCases := []struct {
+		name               string
+		releaseType        string
+		secondaryTypes     []string
+		ignoreSingles      bool
+		ignoreCompilations bool
+		expectedSuppressed bool
+	}{
+		{"Album with EP secondary - should be suppressed when IgnoreSingles=true", "Album", []string{"EP"}, true, false, true},
+		{"Album with EP secondary - should NOT be suppressed when IgnoreSingles=false", "Album", []string{"EP"}, false, false, false},
+		{"Single with EP secondary - should be suppressed when IgnoreSingles=true", "Single", []string{"EP"}, true, false, true},
+		{"Compilation with EP secondary - should be suppressed when IgnoreSingles=true (because EP in secondary counts as Single)", "Compilation", []string{"EP"}, true, false, true},
+	}
+
+	for _, tc := range testCases {
+		tc := tc // capture range variable
+		t.Run(tc.name, func(t *testing.T) {
+			// Create test release
+			release := database.ExternalRelease{
+				Type:           tc.releaseType,
+				SecondaryTypes: tc.secondaryTypes,
+			}
+
+			// Test scanner filter
+			scannerFilter := TypeFilter{
+				IgnoreSingles:      tc.ignoreSingles,
+				IgnoreCompilations: tc.ignoreCompilations,
+			}
+			scannerSuppressed := scannerFilter.suppressed(release)
+
+			// Test musicbrainz filter
+			mbFilter := musicbrainz.FilterOptions{
+				IgnoreSingles:      tc.ignoreSingles,
+				IgnoreCompilations: tc.ignoreCompilations,
+			}
+			mbFiltered := musicbrainz.ApplyTypeToggles([]database.ExternalRelease{release}, mbFilter)
+			mbSuppressed := len(mbFiltered) == 0
+
+			// Both should agree and match expected
+			if scannerSuppressed != mbSuppressed {
+				t.Errorf("Scanner and MusicBrainz filter disagree for %v: scanner=%v, musicbrainz=%v",
+					tc, scannerSuppressed, mbSuppressed)
+			}
+
+			if scannerSuppressed != tc.expectedSuppressed {
+				t.Errorf("Filter returned %v, expected %v for case %v",
+					scannerSuppressed, tc.expectedSuppressed, tc.name)
+			}
+		})
+	}
+}