fix: incorrect sorting in recently added list#125
fix: incorrect sorting in recently added list#125gmcf111 wants to merge 3 commits intodddevid:masterfrom
Conversation
- Add DateTime? created field to Song model - Parse 'created' from JSON response (Navidrome/Subsonic) - Sort by created date (newest first) in Recently Added option - Handle null created by placing at end of list - Add unit test for created field parsing This fixes the issue where 'Recently Added' was just reversing the current list order instead of sorting by actual server-added date.
… list - In _refreshAllDataInBackground(), now fetches all album songs and populates _cachedAllSongs - Uses a Map to deduplicate songs by ID - This fixes the 'No songs found' issue in AllSongsScreen
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a nullable Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/providers/library_provider.dart (1)
305-320:⚠️ Potential issue | 🟠 MajorDo not mark cache as fresh when album-song fetches partially fail.
When any album request fails (Line 311), the flow still sets
_lastCacheUpdate(Line 317) and persists cache (Line 319). That can lock in a partial_cachedAllSongssnapshot and suppress refresh retries for ~6 hours.🔧 Proposed fix
_cachedAllAlbums = allAlbums; final Map<String, Song> songById = {}; + int failedAlbumLoads = 0; for (final album in allAlbums) { try { final albumSongs = await _subsonicService.getAlbumSongs(album.id); for (final song in albumSongs) { songById[song.id] = song; } } catch (e) { + failedAlbumLoads++; debugPrint('Error loading album ${album.id}: $e'); } } _cachedAllSongs = songById.values.toList(); - _lastCacheUpdate = DateTime.now(); - - await _saveCachedData(); + if (failedAlbumLoads == 0) { + _lastCacheUpdate = DateTime.now(); + await _saveCachedData(); + } else { + debugPrint( + 'Background refresh incomplete: $failedAlbumLoads album(s) failed; skipping cache freshness update.', + ); + } notifyListeners();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/library_provider.dart` around lines 305 - 320, The current loop over allAlbums swallows errors from _subsonicService.getAlbumSongs (inside the try/catch) but still sets _cachedAllSongs, _lastCacheUpdate and calls _saveCachedData, which can mark a partial snapshot as fresh; change the logic in the for (final album in allAlbums) block (and its catch) to record when any album fetch fails (e.g., a boolean partialFailure flag or collect failed IDs) and only assign _cachedAllSongs, set _lastCacheUpdate, call _saveCachedData(), and notifyListeners() if no failures occurred; if any failure happened, do not persist or update the cache (instead log and optionally rethrow or return early) so the cache isn’t marked fresh on partial data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/providers/library_provider.dart`:
- Around line 305-320: The current loop over allAlbums swallows errors from
_subsonicService.getAlbumSongs (inside the try/catch) but still sets
_cachedAllSongs, _lastCacheUpdate and calls _saveCachedData, which can mark a
partial snapshot as fresh; change the logic in the for (final album in
allAlbums) block (and its catch) to record when any album fetch fails (e.g., a
boolean partialFailure flag or collect failed IDs) and only assign
_cachedAllSongs, set _lastCacheUpdate, call _saveCachedData(), and
notifyListeners() if no failures occurred; if any failure happened, do not
persist or update the cache (instead log and optionally rethrow or return early)
so the cache isn’t marked fresh on partial data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9a73685-5d68-4ebb-be5b-f1c3c8a38132
📒 Files selected for processing (4)
lib/models/song.dartlib/providers/library_provider.dartlib/screens/all_songs_screen.darttest/models/song_test.dart
Keep the previous cached albums and songs when any album song fetch fails during background refresh. This prevents incomplete libraries from being marked fresh and suppressing retries for several hours.
1 participant
This PR fixes an issue with the sorting of the "recently added" list.
Changes
Corrected the sorting logic for recently added items
Ensured items are ordered properly by time
Improved accuracy and user experience when browsing recent additions
Background
Previously, the list could be incorrectly ordered in certain cases, causing newly added items to not appear in the expected position.
Also, a small request:
In a previous PR, the squash merge caused the contribution attribution to be lost.
If possible, could you please preserve the original commit authorship (e.g., using merge or rebase),
or include Co-authored-by when squashing, so the contribution can be properly credited?
Thanks a lot!
Summary by CodeRabbit
New Features
Improvements
Tests