fix: ensure full scan refreshes all artist stats #4252
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @deluan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request resolves an issue where full library scans were not consistently refreshing all artist statistics, potentially leaving some artist data stale. It introduces a refined mechanism to ensure that during a full scan, all artist stats are updated, while maintaining the existing behavior for quick scans to only refresh stats for artists with detected changes. This change enhances data accuracy and improves the reliability of maintenance operations.
Highlights
- Artist Stats Refresh Logic: The
RefreshStatsmethod in theArtistRepositoryinterface and its implementation now accepts a boolean parameter,allArtists, to explicitly control whether to refresh statistics for all artists or only those with recently updated media files. - Full Scan Behavior: During a full scan, the scanner now explicitly sets
changesDetectedtotrue. This ensures that all necessary maintenance operations, including artist stat refreshes, are automatically triggered, addressing the issue of stale artist statistics after a full scan. - Scanner Integration: The scanner's
fullScanstate is now directly passed to theRefreshStatsmethod. This ensures that full scans correctly trigger a comprehensive refresh of all artist statistics, aligning with the intended behavior. - Testing Utilities: The mock implementation for the
ArtistRepositoryhas been updated to reflect the newRefreshStatsmethod signature, maintaining test compatibility and ensuring proper testing of the updated logic.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that a full library scan refreshes statistics for all artists by introducing an explicit allArtists parameter to RefreshStats, setting changesDetected to true for full scans, and updating implementations and tests accordingly.
- Set
changesDetected = trueduring full scans so all maintenance tasks run - Added
allArtistsparameter toRefreshStatsand updated calls inscanner - Updated the repository interface, persistence implementation, and mock repo to support the new signature
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/mock_artist_repo.go | Added RefreshStats(allArtists bool) and RefreshPlayCounts to the mock repo |
| scanner/scanner.go | Initialized changesDetected atomically and passed fullScan to RefreshStats |
| persistence/artist_repository.go | Modified RefreshStats to accept allArtists and handle both full and partial flows |
| model/artist.go | Updated the ArtistRepository interface to include the new allArtists parameter |
Comments suppressed due to low confidence (3)
persistence/artist_repository.go:300
- [nitpick] The variable name
touchedArtistsQuerySQLis misleading in the full-scan branch since it actually fetches all artist IDs. Consider renaming it toquerySQLor splitting it into separate variables likeallArtistsQuerySQLandtouchedArtistsQuerySQLfor clarity.
touchedArtistsQuerySQL := `SELECT DISTINCT id FROM artist WHERE id <> ''`
scanner/scanner.go:162
- There are no existing tests to verify that
RefreshStatsis called withallArtists=truewhenfullScan=true. Add a unit test forscanAllorrunRefreshStatsto cover this new behavior.
stats, err := s.ds.Artist(ctx).RefreshStats(state.fullScan)
scanner/scanner.go:48
- Moving the
startTimedeclaration before the library fetch changes the duration measurement to include theGetAll()call. If the intent was to measure only the scan phase, consider moving it back to just before the main scan loop.
startTime := time.Now()
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where a full scan didn't refresh all artist statistics. The changes are logical and well-described. The introduction of the allArtists parameter in RefreshStats and forcing changesDetected for full scans are clean solutions. I've made suggestions to improve variable naming for better code clarity and maintainability.
After PR #4059, full scans were not forcing a refresh of all artists. This change ensures that during full scans, all artist stats are refreshed instead of only those with recently updated media files. Changes: - Set changesDetected=true at start of full scans to ensure maintenance operations run - Add allArtists parameter to RefreshStats() method - Pass fullScan state to RefreshStats to control refresh scope - Update mock repository to match new interface Fixes #4246 Related to PR #4059
Signed-off-by: Deluan <deluan@navidrome.org>
2e55fd1 to
9aad72b
Compare
Problem
After PR #4059, full scans were not forcing a refresh of all artists. During a full scan, only artists with recently updated media files would have their stats refreshed, leaving other artists with potentially stale statistics.
Solution
This PR implements a clean solution that ensures all artist stats are refreshed during full scans:
Changes Made
Set
changesDetected = truefor full scans - This ensures all maintenance operations (GC, library stats refresh, etc.) run automatically during full scansAdd
allArtistsparameter toRefreshStats()- Makes the intent explicit:allArtists = true: Refresh stats for all artistsallArtists = false: Refresh stats for touched artists only (original behavior)Pass
fullScanstate toRefreshStats- The scanner now passesstate.fullScanas theallArtistsparameterUpdate mock repository - Updated test mock to match the new interface
Benefits
allArtistsparameter makes it crystal clear what the method doesTesting
Related Issues
Fixes #4246
Related to PR #4059
Behavior
changesDetected = true→ All maintenance runs →RefreshStats(true)→ Refreshes all artistschangesDetectedset only when actual changes detected →RefreshStats(false)→ Refreshes touched artists only