feat: complete task 2 - update FilterReleaseGroups to use centralized filter helper
This commit is contained in:
136
docs/plans/2026-07-21-fix-review-findings.md
Normal file
136
docs/plans/2026-07-21-fix-review-findings.md
Normal file
@@ -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
|
||||
Reference in New Issue
Block a user