Add genre browsing support#147
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Genre domain: model, provider, screens (list + details with pagination/sort/search), routing, playable-by-genre pagination, provider wiring and exports, UI/menu wiring, tests and generated mock updates, plus a small message-overlay padding tweak. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant GenresUI as GenresScreen
participant GenreProv as GenreProvider
participant Network as API
participant DetailsUI as GenreDetailsScreen
participant PlayableProv as PlayableProvider
User->>GenresUI: open /genres
GenresUI->>GenreProv: fetch()
GenreProv->>Network: GET /genres
Network-->>GenreProv: genres[]
GenreProv-->>GenresUI: genres list
User->>GenresUI: tap a Genre
GenresUI->>DetailsUI: navigate(with Genre)
DetailsUI->>PlayableProv: paginateByGenre(genreId, page=1, sort, order)
PlayableProv->>Network: GET /genres/{id}/songs?page=1&sort=...&order=...
Network-->>PlayableProv: playables[], links/meta
PlayableProv-->>DetailsUI: PaginationResult(playables, nextPage)
DetailsUI->>User: render songs (pagination, search, sort)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
phanan
force-pushed
the
feat/genre-support
branch
from
941f2b5 to
bcd8da8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
lib/models/genre.dart (1)
44-45: Consider breaking long line for readability.The genres list spans a long single line. This is a minor style nit.
♻️ Optional formatting improvement
factory Genre.fake({String? id, String? name, int? songCount, num? length}) { final faker = Faker(); - final genres = ['Rock', 'Pop', 'Jazz', 'Classical', 'Hip Hop', 'Electronic', - 'R&B', 'Country', 'Metal', 'Blues', 'Folk', 'Reggae', 'Punk']; + final genres = [ + 'Rock', 'Pop', 'Jazz', 'Classical', 'Hip Hop', 'Electronic', + 'R&B', 'Country', 'Metal', 'Blues', 'Folk', 'Reggae', 'Punk', + ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/models/genre.dart` around lines 44 - 45, The genres list variable 'genres' is on a single long line; split the array literal across multiple lines for readability by placing each genre (or small groups) on its own line within the list literal in the genres declaration (the final genres = [...] assignment), keeping trailing commas and Dart formatting conventions so the formatter produces a clean multiline list.lib/providers/genre_provider.dart (1)
44-54:genreslist and_vaultmay contain different object instances after fetch.When merging, existing entries in
_vaultare updated in-place vialocal.merge(genre), butgenresis assigned the freshly parseditemslist. This means_vault[id]and the corresponding entry ingenresare different object instances.If consumers hold references to objects from
genres, they won't see updates made to_vaultentries (and vice versa). Consider buildinggenresfrom_vaultvalues to ensure consistency:♻️ Proposed fix
for (final genre in items) { final local = _vault[genre.id]; if (local != null) { local.merge(genre); } else { _vault[genre.id] = genre; } } - genres = items; + // Preserve order from API response, but use _vault instances for consistency + genres = items.map((g) => _vault[g.id]!).toList(); notifyListeners();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/genre_provider.dart` around lines 44 - 54, The code assigns genres = items after merging updates into existing objects in _vault, which causes _vault[id] and the list entries in genres to be different instances; instead, after iterating and merging via local.merge(genre) (or inserting new entries), rebuild the genres list from the canonical objects stored in _vault (e.g., map _vault.values to a list) so that both _vault and the genres list reference the same instances and consumers see consistent state changes.test/models/genre_test.dart (2)
40-48: Merge test doesn't verifylengthfield is updated.The
mergemethod updatesname,songCount, andlength, but this test only assertsnameandsongCount. Consider adding an assertion forlengthto ensure complete coverage.Additionally, the two
Genre.fake()instances will have different randomidvalues. While the merge operation still works, semantically merging objects with different IDs may be misleading. Consider using the sameidfor both to better represent the real-world merge scenario.💡 Suggested improvement
group('merge', () { test('updates fields from remote', () { - final local = Genre.fake(name: 'Old', songCount: 1); - final remote = Genre.fake(name: 'New', songCount: 5); + final local = Genre.fake(id: 'rock', name: 'Old', songCount: 1, length: 100); + final remote = Genre.fake(id: 'rock', name: 'New', songCount: 5, length: 500); local.merge(remote); expect(local.name, 'New'); expect(local.songCount, 5); + expect(local.length, 500); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/genre_test.dart` around lines 40 - 48, The test 'merge' should assert that the length field is updated: after calling local.merge(remote) add expect(local.length, remote.length) (or the concrete expected value) to verify length is copied; also construct the two Genre.fakes with the same id (pass the same id into both Genre.fake calls) so the test reflects a real-world merge between the same entity rather than two different random IDs; update references in the test around local, remote, Genre.fake, and merge accordingly.
28-38: Consider adding edge case test for zero length.The
formattedLengthtests cover hours+minutes and minutes-only cases, but don't test whenlengthis 0. This would return'0m', which is worth verifying explicitly.💡 Suggested test case
test('formats zero length', () { final genre = Genre.fake(length: 0); expect(genre.formattedLength, '0m'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/genre_test.dart` around lines 28 - 38, Add an edge-case unit test for zero length in the formattedLength group: create a Genre via Genre.fake(length: 0) and assert that genre.formattedLength equals '0m'. Place the test alongside the existing tests in the group('formattedLength') block to verify Genre.formattedLength handles length == 0 correctly.lib/ui/screens/genres.dart (1)
74-77: Redundant bounds check.The
if (index >= provider.genres.length) return null;check is unnecessary becauseSliverChildBuilderDelegatewithchildCount: provider.genres.lengthalready guaranteesindexwill be within bounds.♻️ Proposed simplification
SliverList( delegate: SliverChildBuilderDelegate( (context, index) { - if (index >= provider.genres.length) return null; final genre = provider.genres[index]; return GenreRow( genre: genre, router: widget.router, ); }, childCount: provider.genres.length, ), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genres.dart` around lines 74 - 77, Remove the redundant bounds check inside the SliverChildBuilderDelegate closure: delete the line `if (index >= provider.genres.length) return null;` and directly use `final genre = provider.genres[index];` (the delegate already has `childCount: provider.genres.length` so SliverChildBuilderDelegate guarantees valid indices). Ensure the closure returns the widget for `genre` as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/providers/genre_provider.dart`:
- Around line 28-34: The GenreProvider constructor subscribes to
AuthProvider.userLoggedOutStream but does not cancel subscriptions on disposal;
add an override of dispose() in GenreProvider that calls unsubscribeAll() (from
the StreamSubscriber mixin) and then calls super.dispose() to ensure the stream
subscription is cancelled when the provider is disposed. Ensure the method
signature matches the provider lifecycle (override void dispose()) and place the
unsubscribeAll() call before calling super.dispose().
In `@lib/ui/screens/genre_details.dart`:
- Line 25: The ScrollController instance named _scrollController is never
disposed; add an override of dispose() in the State class that owns
_scrollController (e.g., the GenreDetails State) and call
_scrollController.dispose() before calling super.dispose() to prevent memory
leaks.
- Around line 46-48: The condition in the FutureBuilder is checking
ConnectionState.active (which only applies to Streams) so that branch is dead;
update the condition that returns PlayableListScreenPlaceholder to use the
correct Future states—e.g., replace the ConnectionState.active check with
ConnectionState.waiting or check snapshot.connectionState !=
ConnectionState.done; modify the if that references snapshot.hasData and
ConnectionState.active in genre_details.dart (around the FutureBuilder handling)
to use snapshot.connectionState == ConnectionState.waiting (or
snapshot.connectionState != ConnectionState.done) so the placeholder shows while
the Future is pending.
- Around line 43-49: The FutureBuilder is recreating the future each build by
calling _fetchSongs(genre) directly; change this by creating a state field (e.g.
_songsFuture) and assign it once in initState or didChangeDependencies (using
the current genre) so the Future is stable across builds, update the
FutureBuilder to use _songsFuture, and when pull-to-refresh or explicit refresh
is needed reassign _songsFuture = _fetchSongs(genre) and call setState to
trigger a reload.
In `@lib/ui/screens/genres.dart`:
- Around line 27-42: fetchData() calls setState after awaiting an async fetch
which can run after the widget is disposed; wrap or gate all setState calls in
fetchData with mounted checks (e.g., check mounted before the initial setState,
before setting _errored in the catch, and before setting _loading = false in
finally) so you never call setState on an unmounted widget — locate the
fetchData function and add mounted guards around the setState calls that update
_errored and _loading and any other state modifications after the await of
context.read<GenreProvider>().fetch().
---
Nitpick comments:
In `@lib/models/genre.dart`:
- Around line 44-45: The genres list variable 'genres' is on a single long line;
split the array literal across multiple lines for readability by placing each
genre (or small groups) on its own line within the list literal in the genres
declaration (the final genres = [...] assignment), keeping trailing commas and
Dart formatting conventions so the formatter produces a clean multiline list.
In `@lib/providers/genre_provider.dart`:
- Around line 44-54: The code assigns genres = items after merging updates into
existing objects in _vault, which causes _vault[id] and the list entries in
genres to be different instances; instead, after iterating and merging via
local.merge(genre) (or inserting new entries), rebuild the genres list from the
canonical objects stored in _vault (e.g., map _vault.values to a list) so that
both _vault and the genres list reference the same instances and consumers see
consistent state changes.
In `@lib/ui/screens/genres.dart`:
- Around line 74-77: Remove the redundant bounds check inside the
SliverChildBuilderDelegate closure: delete the line `if (index >=
provider.genres.length) return null;` and directly use `final genre =
provider.genres[index];` (the delegate already has `childCount:
provider.genres.length` so SliverChildBuilderDelegate guarantees valid indices).
Ensure the closure returns the widget for `genre` as before.
In `@test/models/genre_test.dart`:
- Around line 40-48: The test 'merge' should assert that the length field is
updated: after calling local.merge(remote) add expect(local.length,
remote.length) (or the concrete expected value) to verify length is copied; also
construct the two Genre.fakes with the same id (pass the same id into both
Genre.fake calls) so the test reflects a real-world merge between the same
entity rather than two different random IDs; update references in the test
around local, remote, Genre.fake, and merge accordingly.
- Around line 28-38: Add an edge-case unit test for zero length in the
formattedLength group: create a Genre via Genre.fake(length: 0) and assert that
genre.formattedLength equals '0m'. Place the test alongside the existing tests
in the group('formattedLength') block to verify Genre.formattedLength handles
length == 0 correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aae17d93-de44-496f-a8b1-46d7d3714422
📒 Files selected for processing (13)
lib/main.dartlib/models/genre.dartlib/models/models.dartlib/providers/genre_provider.dartlib/providers/playable_provider.dartlib/providers/providers.dartlib/router.dartlib/ui/screens/genre_details.dartlib/ui/screens/genres.dartlib/ui/screens/library.dartlib/ui/screens/screens.dartlib/ui/widgets/playable_list_header.darttest/models/genre_test.dart
Comment on lines
+28
to
+34
| GenreProvider() { | ||
| subscribe(AuthProvider.userLoggedOutStream.listen((_) { | ||
| genres.clear(); | ||
| _vault.clear(); | ||
| notifyListeners(); | ||
| })); | ||
| } |
There was a problem hiding this comment.
Missing dispose() method to cancel stream subscriptions.
The StreamSubscriber mixin requires calling unsubscribeAll() in the provider's dispose() method to cancel subscriptions and prevent memory leaks. Without this, the subscription to userLoggedOutStream will persist even after the provider is disposed.
🔧 Proposed fix
GenreProvider() {
subscribe(AuthProvider.userLoggedOutStream.listen((_) {
genres.clear();
_vault.clear();
notifyListeners();
}));
}
+@override
+void dispose() {
+ unsubscribeAll();
+ super.dispose();
+}
+
Genre? byId(String id) => _vault[id];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| GenreProvider() { | |
| subscribe(AuthProvider.userLoggedOutStream.listen((_) { | |
| genres.clear(); | |
| _vault.clear(); | |
| notifyListeners(); | |
| })); | |
| } | |
| GenreProvider() { | |
| subscribe(AuthProvider.userLoggedOutStream.listen((_) { | |
| genres.clear(); | |
| _vault.clear(); | |
| notifyListeners(); | |
| })); | |
| } | |
| `@override` | |
| void dispose() { | |
| unsubscribeAll(); | |
| super.dispose(); | |
| } | |
| Genre? byId(String id) => _vault[id]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/providers/genre_provider.dart` around lines 28 - 34, The GenreProvider
constructor subscribes to AuthProvider.userLoggedOutStream but does not cancel
subscriptions on disposal; add an override of dispose() in GenreProvider that
calls unsubscribeAll() (from the StreamSubscriber mixin) and then calls
super.dispose() to ensure the stream subscription is cancelled when the provider
is disposed. Ensure the method signature matches the provider lifecycle
(override void dispose()) and place the unsubscribeAll() call before calling
super.dispose().
Comment on lines
+27
to
+42
| Future<void> fetchData() async { | ||
| if (_loading) return; | ||
|
|
||
| setState(() { | ||
| _errored = false; | ||
| _loading = true; | ||
| }); | ||
|
|
||
| try { | ||
| await context.read<GenreProvider>().fetch(); | ||
| } catch (_) { | ||
| setState(() => _errored = true); | ||
| } finally { | ||
| setState(() => _loading = false); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing mounted checks before setState calls.
fetchData() is an async method that can complete after the widget is disposed (e.g., if the user navigates away during the fetch). Calling setState on an unmounted widget throws an exception.
🔧 Proposed fix
Future<void> fetchData() async {
if (_loading) return;
+ if (!mounted) return;
setState(() {
_errored = false;
_loading = true;
});
try {
await context.read<GenreProvider>().fetch();
} catch (_) {
- setState(() => _errored = true);
+ if (mounted) setState(() => _errored = true);
} finally {
- setState(() => _loading = false);
+ if (mounted) setState(() => _loading = false);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/screens/genres.dart` around lines 27 - 42, fetchData() calls setState
after awaiting an async fetch which can run after the widget is disposed; wrap
or gate all setState calls in fetchData with mounted checks (e.g., check mounted
before the initial setState, before setting _errored in the catch, and before
setting _loading = false in finally) so you never call setState on an unmounted
widget — locate the fetchData function and add mounted guards around the
setState calls that update _errored and _loading and any other state
modifications after the await of context.read<GenreProvider>().fetch().
bcd8da8 to
78eead7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/ui/screens/genre_details.dart (1)
43-53:⚠️ Potential issue | 🔴 CriticalError handling is unreachable—OopsBox will never be shown.
When the Future completes with an error,
snapshot.hasDataisfalseandsnapshot.connectionStateisdone(notactive). The condition on line 46 (!snapshot.hasData || ... == ConnectionState.active) evaluates totrue, returning the placeholder before ever reaching thesnapshot.hasErrorcheck on line 51. Users will see an infinite loading state instead of the retry UI.Additionally, as noted in previous reviews:
- Calling
_fetchSongs(genre)directly infuture:recreates the Future on every build (anti-pattern).ConnectionState.activeapplies to Streams, not Futures—this check is dead code.🔧 Proposed fix: Store the future in state and fix the condition ordering
class _GenreDetailsScreenState extends State<GenreDetailsScreen> { var _searchQuery = ''; var cover = CoverImageStack(playables: []); final _scrollController = ScrollController(); + late Future<List<Playable>> _songsFuture; + Genre? _genre; - Future<List<Playable>> _fetchSongs(Genre genre, {bool forceRefresh = false}) { + Future<List<Playable>> _fetchSongs({bool forceRefresh = false}) { return context .read<PlayableProvider>() - .fetchForGenre(genre.id, forceRefresh: forceRefresh); + .fetchForGenre(_genre!.id, forceRefresh: forceRefresh); + } + + `@override` + void didChangeDependencies() { + super.didChangeDependencies(); + final genre = ModalRoute.of(context)!.settings.arguments as Genre; + if (_genre == null) { + _genre = genre; + _songsFuture = _fetchSongs(); + } } `@override` Widget build(BuildContext context) { - final genre = ModalRoute.of(context)!.settings.arguments as Genre; // ... return Scaffold( body: GradientDecoratedContainer( child: FutureBuilder( - future: _fetchSongs(genre), + future: _songsFuture, builder: (_, AsyncSnapshot<List<Playable>> snapshot) { - if (!snapshot.hasData || - snapshot.connectionState == ConnectionState.active) { + if (snapshot.connectionState == ConnectionState.waiting) { return const PlayableListScreenPlaceholder(); } if (snapshot.hasError) { - return OopsBox(onRetry: () => setState(() {})); + return OopsBox( + onRetry: () => setState(() { + _songsFuture = _fetchSongs(); + }), + ); }And update pull-to-refresh:
onRefresh: () async { - await _fetchSongs(genre, forceRefresh: true); - if (mounted) setState(() {}); + setState(() { + _songsFuture = _fetchSongs(forceRefresh: true); + }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genre_details.dart` around lines 43 - 53, The FutureBuilder currently calls _fetchSongs(genre) directly and checks for !snapshot.hasData || connectionState == ConnectionState.active before checking snapshot.hasError, which makes OopsBox unreachable and recreates the Future on every build; fix by storing the future in state (e.g., a _songsFuture field set in initState and when genre changes) and use that stored future in FutureBuilder, change the builder order to return OopsBox when snapshot.hasError first, then return PlayableListScreenPlaceholder while snapshot.connectionState != ConnectionState.done or !snapshot.hasData (remove the ConnectionState.active check since this is a Future), and update pull-to-refresh to refresh by assigning a new _songsFuture = _fetchSongs(genre) inside setState so the FutureBuilder reruns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/ui/screens/genre_details.dart`:
- Around line 43-53: The FutureBuilder currently calls _fetchSongs(genre)
directly and checks for !snapshot.hasData || connectionState ==
ConnectionState.active before checking snapshot.hasError, which makes OopsBox
unreachable and recreates the Future on every build; fix by storing the future
in state (e.g., a _songsFuture field set in initState and when genre changes)
and use that stored future in FutureBuilder, change the builder order to return
OopsBox when snapshot.hasError first, then return PlayableListScreenPlaceholder
while snapshot.connectionState != ConnectionState.done or !snapshot.hasData
(remove the ConnectionState.active check since this is a Future), and update
pull-to-refresh to refresh by assigning a new _songsFuture = _fetchSongs(genre)
inside setState so the FutureBuilder reruns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2c76a04-74d6-4ac8-a173-49bedb0286ed
📒 Files selected for processing (13)
lib/main.dartlib/models/genre.dartlib/models/models.dartlib/providers/genre_provider.dartlib/providers/playable_provider.dartlib/providers/providers.dartlib/router.dartlib/ui/screens/genre_details.dartlib/ui/screens/genres.dartlib/ui/screens/library.dartlib/ui/screens/screens.dartlib/ui/widgets/playable_list_header.darttest/models/genre_test.dart
✅ Files skipped from review due to trivial changes (7)
- lib/main.dart
- lib/providers/providers.dart
- lib/models/models.dart
- lib/ui/screens/library.dart
- lib/ui/screens/screens.dart
- test/models/genre_test.dart
- lib/models/genre.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/ui/widgets/playable_list_header.dart
- lib/router.dart
- lib/providers/playable_provider.dart
- lib/providers/genre_provider.dart
phanan
force-pushed
the
feat/genre-support
branch
2 times, most recently
from
0077456 to
f41521c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/ui/screens/genres.dart (1)
27-42:⚠️ Potential issue | 🟠 MajorMissing
mountedchecks beforesetStatecalls after async operation.The
fetchData()method callssetStateat lines 38 and 40 after awaiting an async operation. If the widget is disposed during the fetch (e.g., user navigates away), this will throw an exception.🔧 Proposed fix
Future<void> fetchData() async { if (_loading) return; + if (!mounted) return; setState(() { _errored = false; _loading = true; }); try { await context.read<GenreProvider>().fetch(); } catch (_) { - setState(() => _errored = true); + if (mounted) setState(() => _errored = true); } finally { - setState(() => _loading = false); + if (mounted) setState(() => _loading = false); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genres.dart` around lines 27 - 42, fetchData() calls setState after awaiting an async fetch which can run after the State is disposed; guard those post-await setState calls by checking the State's mounted flag. Update the catch and finally blocks inside fetchData() (the setState that sets _errored and the setState that sets _loading = false) to first verify mounted (e.g., if (!mounted) return or if (!mounted) break/skip calling setState) so you only call setState when the State instance is still mounted.
🧹 Nitpick comments (3)
lib/ui/screens/genre_details.dart (2)
179-185: PreferSizedBoxoverContainerfor sizing-only widgets.When only dimensions are needed without decoration or other Container features,
SizedBoxis more semantic and slightly more performant.♻️ Proposed fix
if (_loading) SliverToBoxAdapter( - child: Container( + child: SizedBox( height: 72, child: const Center(child: Spinner(size: 16)), ), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genre_details.dart` around lines 179 - 185, Replace the sizing-only Container inside the SliverToBoxAdapter with a SizedBox to be more semantic and slightly more performant: locate the conditional branch that checks _loading and returns SliverToBoxAdapter containing Container(height: 72, child: Center(child: Spinner(size: 16))). Change that Container to a SizedBox with the same height and the same child (Center -> Spinner) so the layout and behavior remain identical.
148-156: Reset_coverwhen sort changes to maintain consistency with refresh behavior.When the sort configuration changes,
_songsis cleared and pagination resets, but_coverretains stale images. The_refresh()method at line 80 correctly resets_cover. For consistency, consider resetting it here too.♻️ Proposed fix
onMenuItemSelected: (config) { AppState.set('genre.sort', config); setState(() { _sortConfig = config; _songs.clear(); _nextPage = 1; + _cover = CoverImageStack(playables: []); }); _fetchMore(); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genre_details.dart` around lines 148 - 156, When the sort changes in the onMenuItemSelected handler (where AppState.set('genre.sort') is called and setState updates _sortConfig, clears _songs, and resets _nextPage), also reset the cached cover by clearing _cover the same way _refresh() does; add a statement in that setState block to reset _cover (e.g., set _cover to null or an empty value) before calling _fetchMore() so the cover image doesn't remain stale after a sort change.lib/ui/screens/genres.dart (1)
65-66: Inconsistent refresh handling betweenfetchData()andprovider.refresh().The pull-to-refresh uses
provider.refresh()directly, bypassing the local_loading/_erroredstate management infetchData(). Ifprovider.refresh()throws, the error state won't be captured locally. Consider wrapping the refresh call to maintain consistent error handling.♻️ Proposed fix
child: PullToRefresh( - onRefresh: provider.refresh, + onRefresh: () async { + try { + await provider.refresh(); + } catch (_) { + if (mounted) setState(() => _errored = true); + } + }, child: CustomScrollView(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genres.dart` around lines 65 - 66, The PullToRefresh onRefresh currently calls provider.refresh() directly which bypasses the local _loading/_errored handling in fetchData(); change onRefresh to call a wrapper that mirrors fetchData's state handling (or call fetchData() itself) so _loading is set true before awaiting provider.refresh(), catch errors to set _errored = true (and clear it on success), and finally set _loading = false; update the onRefresh reference (currently bound to provider.refresh) to this new method to ensure errors thrown by provider.refresh() are captured and UI state is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/screens/genre_details.dart`:
- Around line 47-74: In _fetchMore, avoid calling setState after the await by
checking the widget is still mounted: after the paginateByGenre await return
early if (!mounted); then perform the setState that updates _songs, _nextPage,
_initialLoading and _cover; likewise in the catch and finally blocks, guard the
setState calls with a mounted check (or return early) so setState is never
invoked on an unmounted widget; reference _fetchMore, paginateByGenre, and the
setState calls that update _songs/_cover/_errored/_loading when applying the
changes.
---
Duplicate comments:
In `@lib/ui/screens/genres.dart`:
- Around line 27-42: fetchData() calls setState after awaiting an async fetch
which can run after the State is disposed; guard those post-await setState calls
by checking the State's mounted flag. Update the catch and finally blocks inside
fetchData() (the setState that sets _errored and the setState that sets _loading
= false) to first verify mounted (e.g., if (!mounted) return or if (!mounted)
break/skip calling setState) so you only call setState when the State instance
is still mounted.
---
Nitpick comments:
In `@lib/ui/screens/genre_details.dart`:
- Around line 179-185: Replace the sizing-only Container inside the
SliverToBoxAdapter with a SizedBox to be more semantic and slightly more
performant: locate the conditional branch that checks _loading and returns
SliverToBoxAdapter containing Container(height: 72, child: Center(child:
Spinner(size: 16))). Change that Container to a SizedBox with the same height
and the same child (Center -> Spinner) so the layout and behavior remain
identical.
- Around line 148-156: When the sort changes in the onMenuItemSelected handler
(where AppState.set('genre.sort') is called and setState updates _sortConfig,
clears _songs, and resets _nextPage), also reset the cached cover by clearing
_cover the same way _refresh() does; add a statement in that setState block to
reset _cover (e.g., set _cover to null or an empty value) before calling
_fetchMore() so the cover image doesn't remain stale after a sort change.
In `@lib/ui/screens/genres.dart`:
- Around line 65-66: The PullToRefresh onRefresh currently calls
provider.refresh() directly which bypasses the local _loading/_errored handling
in fetchData(); change onRefresh to call a wrapper that mirrors fetchData's
state handling (or call fetchData() itself) so _loading is set true before
awaiting provider.refresh(), catch errors to set _errored = true (and clear it
on success), and finally set _loading = false; update the onRefresh reference
(currently bound to provider.refresh) to this new method to ensure errors thrown
by provider.refresh() are captured and UI state is consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bede07b2-de02-4ef7-8afc-47a130a650c8
📒 Files selected for processing (13)
lib/main.dartlib/models/genre.dartlib/models/models.dartlib/providers/genre_provider.dartlib/providers/playable_provider.dartlib/providers/providers.dartlib/router.dartlib/ui/screens/genre_details.dartlib/ui/screens/genres.dartlib/ui/screens/library.dartlib/ui/screens/screens.dartlib/ui/widgets/playable_list_header.darttest/models/genre_test.dart
✅ Files skipped from review due to trivial changes (6)
- lib/providers/providers.dart
- lib/main.dart
- lib/models/models.dart
- lib/ui/screens/screens.dart
- lib/ui/widgets/playable_list_header.dart
- lib/ui/screens/library.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/router.dart
- lib/providers/playable_provider.dart
- test/models/genre_test.dart
- lib/providers/genre_provider.dart
- lib/models/genre.dart
phanan
force-pushed
the
feat/genre-support
branch
from
f41521c to
bd5ddcd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/ui/screens/genres.dart (1)
27-41:⚠️ Potential issue | 🟠 MajorAdd
mountedguards for async state updates infetchData().After the
awaiton Line 36,setStateon Lines 38 and 40 can run after disposal and crash.🔧 Proposed fix
Future<void> fetchData() async { if (_loading) return; + if (!mounted) return; setState(() { _errored = false; _loading = true; }); try { await context.read<GenreProvider>().fetch(); } catch (_) { - setState(() => _errored = true); + if (mounted) setState(() => _errored = true); } finally { - setState(() => _loading = false); + if (mounted) setState(() => _loading = false); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genres.dart` around lines 27 - 41, The async fetchData() may call setState after an await which can run after the widget is disposed; update fetchData to guard post-await state updates by checking mounted: after the await context.read<GenreProvider>().fetch() (and in the catch/finally blocks) return early if mounted is false or wrap the setState(...) calls in if (!mounted) return / if (mounted) setState(...); ensure you still set the initial _loading/_errored state before the await as-is and only add mounted checks for the setState calls that occur after the await in fetchData().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/screens/genre_details.dart`:
- Around line 76-83: The refresh flow clears _songs before attempting _fetchMore
so a failed refresh leaves an empty list with no retry UI; change _refresh to
NOT clear _songs/_cover/_nextPage before calling _fetchMore (attempt fetch
first, then on success reset/replace state), and update the build logic that
currently only shows the error UI when _initialLoading is true to instead render
the error block when _errored is true and displayedSongs.isEmpty (so the
SliverPlayableList will show the retry/error UI when a refresh fails). Ensure
you touch the _refresh method, _fetchMore error handling, and the build
condition that references
_initialLoading/_errored/displayedSongs/SliverPlayableList.
- Around line 47-73: The async method _fetchMore currently calls setState after
await points and can trigger "setState called after dispose"; update _fetchMore
to check the widget's mounted flag before any post-await setState calls (e.g.,
after awaiting paginateByGenre and inside catch/finally). Specifically, in
_fetchMore (the method using _loading, _nextPage, _songs, _initialLoading,
_cover, and setting _errored), either return early if (!mounted) before calling
setState or wrap those setState blocks with if (!mounted) return / if (mounted)
setState(...) so no setState runs after the widget is disposed.
In `@lib/ui/screens/genres.dart`:
- Around line 65-67: Replace the direct onRefresh: provider.refresh call with a
screen-level async handler that calls GenreProvider.refresh inside a try/catch
so refresh failures can set the provider error state and preserve existing data;
add a public setter method on GenreProvider (e.g., setErrored(bool) or
markErrored()) to update the private _errored flag from the UI, and update
GenreProvider.refresh to avoid clearing existing data before the network call
(only clear/replace after a successful fetch) so the catch block can call
provider.setErrored(true) and allow the retry UI to render.
---
Duplicate comments:
In `@lib/ui/screens/genres.dart`:
- Around line 27-41: The async fetchData() may call setState after an await
which can run after the widget is disposed; update fetchData to guard post-await
state updates by checking mounted: after the await
context.read<GenreProvider>().fetch() (and in the catch/finally blocks) return
early if mounted is false or wrap the setState(...) calls in if (!mounted)
return / if (mounted) setState(...); ensure you still set the initial
_loading/_errored state before the await as-is and only add mounted checks for
the setState calls that occur after the await in fetchData().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f87ce4a-1bb5-4baa-84ab-f42437f53d35
📒 Files selected for processing (14)
lib/main.dartlib/models/genre.dartlib/models/models.dartlib/providers/genre_provider.dartlib/providers/playable_provider.dartlib/providers/providers.dartlib/router.dartlib/ui/screens/genre_details.dartlib/ui/screens/genres.dartlib/ui/screens/library.dartlib/ui/screens/screens.dartlib/ui/widgets/message_overlay.dartlib/ui/widgets/playable_list_header.darttest/models/genre_test.dart
✅ Files skipped from review due to trivial changes (7)
- lib/models/models.dart
- lib/providers/providers.dart
- lib/ui/widgets/message_overlay.dart
- lib/main.dart
- lib/ui/widgets/playable_list_header.dart
- test/models/genre_test.dart
- lib/models/genre.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/ui/screens/screens.dart
- lib/router.dart
- lib/providers/playable_provider.dart
- lib/providers/genre_provider.dart
phanan
force-pushed
the
feat/genre-support
branch
from
bd5ddcd to
c83e9da
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lib/ui/screens/genre_details.dart (1)
52-73:⚠️ Potential issue | 🟠 MajorGuard
setStateafter async await in_fetchMore().After the await on Line [53],
setStateis called intry/catch/finallywithout a mounted guard. Navigating away mid-request can triggersetState()on a disposed state.🔧 Suggested fix
Future<void> _fetchMore() async { if (_loading || _nextPage == null) return; setState(() => _loading = true); try { final result = await context.read<PlayableProvider>().paginateByGenre( _genre.id, page: _nextPage!, sort: _sortConfig.field, order: _sortConfig.order, ); + if (!mounted) return; setState(() { _songs = [..._songs, ...result.items].toSet().toList(); _nextPage = result.nextPage; _initialLoading = false; if (_cover.isEmpty && _songs.isNotEmpty) { _cover = CoverImageStack(playables: _songs); } }); } catch (_) { - setState(() => _errored = true); + if (mounted) setState(() => _errored = true); } finally { - setState(() => _loading = false); + if (mounted) setState(() => _loading = false); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genre_details.dart` around lines 52 - 73, The async pagination call in _fetchMore (context.read<PlayableProvider>().paginateByGenre(...)) performs setState on _songs, _nextPage, _initialLoading, _cover, _errored and _loading after awaiting; guard those setState calls with a mounted check to avoid setState on a disposed State. After the await return of paginateByGenre (and inside both the catch and finally blocks) either early-return if (!mounted) or wrap each setState in if (mounted) { ... } so updates to _songs, _nextPage, _initialLoading, _cover, _errored and _loading only occur when the widget is still mounted.lib/providers/genre_provider.dart (1)
28-34:⚠️ Potential issue | 🟠 MajorAdd
dispose()to unsubscribe stream listeners.At Line [28]–Line [34], the provider subscribes to
AuthProvider.userLoggedOutStreambut never callsunsubscribeAll()on disposal, which can retain stale subscriptions.🔧 Proposed fix
class GenreProvider with ChangeNotifier, StreamSubscriber { @@ GenreProvider() { subscribe(AuthProvider.userLoggedOutStream.listen((_) { genres.clear(); _vault.clear(); notifyListeners(); })); } + + `@override` + void dispose() { + unsubscribeAll(); + super.dispose(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/genre_provider.dart` around lines 28 - 34, The GenreProvider registers a listener via subscribe(...) to AuthProvider.userLoggedOutStream but never removes it on teardown; add an override of dispose() in the GenreProvider class that calls unsubscribeAll() (to remove subscriptions created by subscribe), then calls super.dispose(); ensure dispose remains public and that existing cleanup (genres.clear(), _vault.clear(), notifyListeners()) still runs if needed before or inside the stream callback but not in dispose itself unless appropriate.
🧹 Nitpick comments (1)
lib/providers/genre_provider.dart (1)
17-26: Consider notifying listeners when sort state changes.At Line [17]–Line [26],
sortField/sortOrderare mutable public state but updates do not triggernotifyListeners(). If UI reads these values reactively, it won’t rebuild on change.♻️ Suggested refactor
set sortField(String field) { - if (field != _sortField) { - _sortOrder = SortOrder.asc; - } - _sortField = field; + if (field == _sortField) return; + _sortField = field; + _sortOrder = SortOrder.asc; + notifyListeners(); } set sortOrder(SortOrder order) { + if (order == _sortOrder) return; _sortOrder = order; + notifyListeners(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/genre_provider.dart` around lines 17 - 26, The setters sortField and sortOrder currently mutate state without notifying listeners; update them to call notifyListeners() after state changes (for sortField, only when the incoming field differs: set _sortOrder = SortOrder.asc, update _sortField, then notifyListeners(); for sortOrder, check if order != _sortOrder before assigning and calling notifyListeners()) so UI consumers of the GenreProvider rebuild reactively when sort state changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/providers/genre_provider.dart`:
- Around line 44-54: The fetch logic currently merges and adds entries into the
in-memory map _vault for each genre in items but never removes entries that were
deleted server-side, causing byId() to return stale genres; update the block
that iterates items (where _vault, items, genres are used and local.merge(genre)
is called) to compute the set of incoming IDs, remove any keys from _vault that
are not present in that set before or after merging, and then assign genres and
call notifyListeners() so _vault reflects the server state exactly.
In `@lib/ui/screens/genre_details.dart`:
- Around line 47-58: The fetches can apply stale in-flight responses when sort
changes; add a simple request versioning token (e.g., a private int
_requestVersion on the state) that you increment whenever sort or pagination
mode changes, capture its value at the start of _fetchMore (and the other
pagination/initial fetch method that updates _songs/_nextPage), and after
awaiting the provider call verify the captured token still equals
_requestVersion before mutating _songs/_nextPage/_loading; if it doesn’t match,
discard the response. Ensure you increment _requestVersion when _sortConfig
changes and use the same token-check pattern in both _fetchMore and the other
fetch method to prevent mixing page cursors across sort modes.
---
Duplicate comments:
In `@lib/providers/genre_provider.dart`:
- Around line 28-34: The GenreProvider registers a listener via subscribe(...)
to AuthProvider.userLoggedOutStream but never removes it on teardown; add an
override of dispose() in the GenreProvider class that calls unsubscribeAll() (to
remove subscriptions created by subscribe), then calls super.dispose(); ensure
dispose remains public and that existing cleanup (genres.clear(),
_vault.clear(), notifyListeners()) still runs if needed before or inside the
stream callback but not in dispose itself unless appropriate.
In `@lib/ui/screens/genre_details.dart`:
- Around line 52-73: The async pagination call in _fetchMore
(context.read<PlayableProvider>().paginateByGenre(...)) performs setState on
_songs, _nextPage, _initialLoading, _cover, _errored and _loading after
awaiting; guard those setState calls with a mounted check to avoid setState on a
disposed State. After the await return of paginateByGenre (and inside both the
catch and finally blocks) either early-return if (!mounted) or wrap each
setState in if (mounted) { ... } so updates to _songs, _nextPage,
_initialLoading, _cover, _errored and _loading only occur when the widget is
still mounted.
---
Nitpick comments:
In `@lib/providers/genre_provider.dart`:
- Around line 17-26: The setters sortField and sortOrder currently mutate state
without notifying listeners; update them to call notifyListeners() after state
changes (for sortField, only when the incoming field differs: set _sortOrder =
SortOrder.asc, update _sortField, then notifyListeners(); for sortOrder, check
if order != _sortOrder before assigning and calling notifyListeners()) so UI
consumers of the GenreProvider rebuild reactively when sort state changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 411b74af-adbf-481a-93db-8b3b63718f84
📒 Files selected for processing (21)
lib/main.dartlib/models/genre.dartlib/models/models.dartlib/providers/genre_provider.dartlib/providers/playable_provider.dartlib/providers/providers.dartlib/router.dartlib/ui/screens/genre_details.dartlib/ui/screens/genres.dartlib/ui/screens/library.dartlib/ui/screens/screens.dartlib/ui/widgets/message_overlay.dartlib/ui/widgets/playable_list_header.darttest/models/genre_test.darttest/providers/download_sync_provider_test.mocks.darttest/providers/recently_played_provider_test.mocks.darttest/ui/screens/genres_screen_test.darttest/ui/screens/genres_screen_test.mocks.darttest/ui/widgets/album_card_test.mocks.darttest/ui/widgets/artist_card_test.mocks.darttest/ui/widgets/song_cache_icon_test.mocks.dart
✅ Files skipped from review due to trivial changes (8)
- lib/providers/providers.dart
- lib/models/models.dart
- lib/ui/screens/screens.dart
- lib/ui/widgets/message_overlay.dart
- lib/ui/widgets/playable_list_header.dart
- test/models/genre_test.dart
- test/ui/screens/genres_screen_test.mocks.dart
- lib/models/genre.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/main.dart
- lib/ui/screens/library.dart
- lib/router.dart
- lib/providers/playable_provider.dart
- lib/ui/screens/genres.dart
Comment on lines
+44
to
+54
| for (final genre in items) { | ||
| final local = _vault[genre.id]; | ||
| if (local != null) { | ||
| local.merge(genre); | ||
| } else { | ||
| _vault[genre.id] = genre; | ||
| } | ||
| } | ||
|
|
||
| genres = items; | ||
| notifyListeners(); |
There was a problem hiding this comment.
Prune removed genres from _vault during fetch.
At Line [44]–Line [54], items are merged/added but never removed from _vault, so byId() can return genres that no longer exist server-side after subsequent fetches.
🔧 Proposed fix
Future<void> fetch() async {
final res = await get('genres');
final items = (res as List)
.map<Genre>((json) => Genre.fromJson(json))
.toList();
+ final incomingIds = <String>{};
for (final genre in items) {
+ incomingIds.add(genre.id);
final local = _vault[genre.id];
if (local != null) {
local.merge(genre);
} else {
_vault[genre.id] = genre;
}
}
+ _vault.removeWhere((id, _) => !incomingIds.contains(id));
genres = items;
notifyListeners();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (final genre in items) { | |
| final local = _vault[genre.id]; | |
| if (local != null) { | |
| local.merge(genre); | |
| } else { | |
| _vault[genre.id] = genre; | |
| } | |
| } | |
| genres = items; | |
| notifyListeners(); | |
| final incomingIds = <String>{}; | |
| for (final genre in items) { | |
| incomingIds.add(genre.id); | |
| final local = _vault[genre.id]; | |
| if (local != null) { | |
| local.merge(genre); | |
| } else { | |
| _vault[genre.id] = genre; | |
| } | |
| } | |
| _vault.removeWhere((id, _) => !incomingIds.contains(id)); | |
| genres = items; | |
| notifyListeners(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/providers/genre_provider.dart` around lines 44 - 54, The fetch logic
currently merges and adds entries into the in-memory map _vault for each genre
in items but never removes entries that were deleted server-side, causing byId()
to return stale genres; update the block that iterates items (where _vault,
items, genres are used and local.merge(genre) is called) to compute the set of
incoming IDs, remove any keys from _vault that are not present in that set
before or after merging, and then assign genres and call notifyListeners() so
_vault reflects the server state exactly.
Comment on lines
+47
to
+58
| Future<void> _fetchMore() async { | ||
| if (_loading || _nextPage == null) return; | ||
|
|
||
| setState(() => _loading = true); | ||
|
|
||
| try { | ||
| final result = await context.read<PlayableProvider>().paginateByGenre( | ||
| _genre.id, | ||
| page: _nextPage!, | ||
| sort: _sortConfig.field, | ||
| order: _sortConfig.order, | ||
| ); |
There was a problem hiding this comment.
Sort changes can apply stale fetch results and corrupt pagination state.
If a request is already loading, Line [48] drops the new sorted fetch, but the old in-flight response still updates _songs/_nextPage. This can mix page cursors across sort modes.
🔧 Suggested fix (request versioning)
class _GenreDetailsScreenState extends State<GenreDetailsScreen> {
+ int _requestVersion = 0;
- Future<void> _fetchMore() async {
+ Future<void> _fetchMore({int? requestVersion}) async {
if (_loading || _nextPage == null) return;
+ final currentVersion = requestVersion ?? _requestVersion;
setState(() => _loading = true);
try {
final result = await context.read<PlayableProvider>().paginateByGenre(
_genre.id,
page: _nextPage!,
sort: _sortConfig.field,
order: _sortConfig.order,
);
+ if (!mounted || currentVersion != _requestVersion) return;
setState(() {
_songs = [..._songs, ...result.items].toSet().toList();
_nextPage = result.nextPage;
_initialLoading = false;
});
} catch (_) {
- setState(() => _errored = true);
+ if (mounted && currentVersion == _requestVersion) {
+ setState(() => _errored = true);
+ }
} finally {
- setState(() => _loading = false);
+ if (mounted && currentVersion == _requestVersion) {
+ setState(() => _loading = false);
+ }
}
} onMenuItemSelected: (config) {
AppState.set('genre.sort', config);
setState(() {
+ _requestVersion++;
_sortConfig = config;
_songs.clear();
_nextPage = 1;
});
- _fetchMore();
+ _fetchMore(requestVersion: _requestVersion);
},Also applies to: 148-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/screens/genre_details.dart` around lines 47 - 58, The fetches can
apply stale in-flight responses when sort changes; add a simple request
versioning token (e.g., a private int _requestVersion on the state) that you
increment whenever sort or pagination mode changes, capture its value at the
start of _fetchMore (and the other pagination/initial fetch method that updates
_songs/_nextPage), and after awaiting the provider call verify the captured
token still equals _requestVersion before mutating _songs/_nextPage/_loading; if
it doesn’t match, discard the response. Ensure you increment _requestVersion
when _sortConfig changes and use the same token-check pattern in both _fetchMore
and the other fetch method to prevent mixing page cursors across sort modes.
c83e9da to
7e18f21
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/providers/genre_provider.dart (1)
28-34:⚠️ Potential issue | 🟠 MajorAdd
dispose()to unsubscribe stream listeners.Line 29 creates a stream subscription via
subscribe(...), but the provider never callsunsubscribeAll(). This can leak listeners and keep stale callbacks alive after disposal.🔧 Proposed fix
class GenreProvider with ChangeNotifier, StreamSubscriber { @@ GenreProvider() { subscribe(AuthProvider.userLoggedOutStream.listen((_) { genres.clear(); _vault.clear(); notifyListeners(); })); } + + `@override` + void dispose() { + unsubscribeAll(); + super.dispose(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/genre_provider.dart` around lines 28 - 34, The GenreProvider constructor calls subscribe(AuthProvider.userLoggedOutStream.listen(...)) but never removes subscriptions on disposal, causing leaks; add an override of dispose() in the GenreProvider class that calls unsubscribeAll() (or iterates stored Subscription objects to cancel them) and then calls super.dispose(), ensuring the stream listener created in GenreProvider() is unsubscribed when the provider is disposed; update any constructor comment or class doc if needed to note that subscribers are cleaned up in dispose().lib/ui/screens/genre_details.dart (1)
78-85:⚠️ Potential issue | 🟠 MajorRefresh failure can leave a blank screen without recovery UI.
Lines 79-83 clear data before fetch; if fetch fails,
_erroredbecomes true but the error block at Line 108 is skipped once_initialLoadingis false. Users can be left with an empty list and no retry affordance.🔧 Proposed fix
Future<void> _refresh() async { setState(() { + _errored = false; _songs.clear(); _nextPage = 1; _cover = CoverImageStack(playables: []); }); await _fetchMore(); } @@ Widget build(BuildContext context) { if (_initialLoading) { return Scaffold( body: GradientDecoratedContainer( child: _errored ? OopsBox(onRetry: _fetchMore) : const PlayableListScreenPlaceholder(), ), ); } + + if (_errored && _songs.isEmpty) { + return Scaffold( + body: GradientDecoratedContainer( + child: OopsBox(onRetry: _refresh), + ), + ); + }Also applies to: 106-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genre_details.dart` around lines 78 - 85, In _refresh, don't clear the visible state before a successful fetch; instead snapshot _songs, _nextPage and _cover, call _fetchMore (or expose a private helper like _fetchPage) and only update state with the new values on success; on failure restore the snapshot and set _errored = true (and call setState) so the error UI and retry affordance render even when _initialLoading is false; apply the same approach to the initial-loading code paths around _initialLoading/_errored (the block currently at Lines 106-116) so you preserve existing content until the network fetch completes or fails.
🧹 Nitpick comments (1)
test/ui/screens/genres_screen_test.dart (1)
27-65: Strengthen tests by assertingfetch()is actually invoked.The tests currently validate rendered output but don’t verify the initial load call. Adding
verify(genreProviderMock.fetch()).called(1);(per test) will lock the screen/provider interaction contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/screens/genres_screen_test.dart` around lines 27 - 65, Add verification that the provider's fetch method is invoked in each widget test by calling verify(genreProviderMock.fetch()).called(1); after the widget has been pumped and settled; update the three tests ("shows genre list", "shows singular \"song\" for count of 1", "shows Unknown Genre for empty name") to include this verify step so the contract between the screen and genreProviderMock (genreProviderMock.genres and genreProviderMock.fetch()) is asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/models/genre.dart`:
- Around line 16-23: The deserialization in Genre.fromJson currently falls back
to an empty string for id which can collapse multiple malformed records into the
same identity; change Genre.fromJson to compute id = json['id'] ??
json['public_id'] and if that value is null or empty throw a descriptive
FormatException (or assert) rather than returning '' so malformed payloads fail
fast; update any calling code/tests if they expect silent fallbacks.
---
Duplicate comments:
In `@lib/providers/genre_provider.dart`:
- Around line 28-34: The GenreProvider constructor calls
subscribe(AuthProvider.userLoggedOutStream.listen(...)) but never removes
subscriptions on disposal, causing leaks; add an override of dispose() in the
GenreProvider class that calls unsubscribeAll() (or iterates stored Subscription
objects to cancel them) and then calls super.dispose(), ensuring the stream
listener created in GenreProvider() is unsubscribed when the provider is
disposed; update any constructor comment or class doc if needed to note that
subscribers are cleaned up in dispose().
In `@lib/ui/screens/genre_details.dart`:
- Around line 78-85: In _refresh, don't clear the visible state before a
successful fetch; instead snapshot _songs, _nextPage and _cover, call _fetchMore
(or expose a private helper like _fetchPage) and only update state with the new
values on success; on failure restore the snapshot and set _errored = true (and
call setState) so the error UI and retry affordance render even when
_initialLoading is false; apply the same approach to the initial-loading code
paths around _initialLoading/_errored (the block currently at Lines 106-116) so
you preserve existing content until the network fetch completes or fails.
---
Nitpick comments:
In `@test/ui/screens/genres_screen_test.dart`:
- Around line 27-65: Add verification that the provider's fetch method is
invoked in each widget test by calling
verify(genreProviderMock.fetch()).called(1); after the widget has been pumped
and settled; update the three tests ("shows genre list", "shows singular
\"song\" for count of 1", "shows Unknown Genre for empty name") to include this
verify step so the contract between the screen and genreProviderMock
(genreProviderMock.genres and genreProviderMock.fetch()) is asserted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 681dd282-48fe-4292-9fd6-e70c3b3d59c5
📒 Files selected for processing (21)
lib/main.dartlib/models/genre.dartlib/models/models.dartlib/providers/genre_provider.dartlib/providers/playable_provider.dartlib/providers/providers.dartlib/router.dartlib/ui/screens/genre_details.dartlib/ui/screens/genres.dartlib/ui/screens/library.dartlib/ui/screens/screens.dartlib/ui/widgets/message_overlay.dartlib/ui/widgets/playable_list_header.darttest/models/genre_test.darttest/providers/download_sync_provider_test.mocks.darttest/providers/recently_played_provider_test.mocks.darttest/ui/screens/genres_screen_test.darttest/ui/screens/genres_screen_test.mocks.darttest/ui/widgets/album_card_test.mocks.darttest/ui/widgets/artist_card_test.mocks.darttest/ui/widgets/song_cache_icon_test.mocks.dart
✅ Files skipped from review due to trivial changes (10)
- lib/models/models.dart
- lib/ui/screens/library.dart
- lib/ui/screens/screens.dart
- lib/providers/providers.dart
- lib/ui/widgets/message_overlay.dart
- lib/ui/widgets/playable_list_header.dart
- test/models/genre_test.dart
- test/providers/download_sync_provider_test.mocks.dart
- lib/ui/screens/genres.dart
- test/ui/screens/genres_screen_test.mocks.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/main.dart
- lib/router.dart
Comment on lines
+16
to
+23
| factory Genre.fromJson(Map<String, dynamic> json) { | ||
| return Genre( | ||
| id: json['id'] ?? json['public_id'] ?? '', | ||
| name: json['name'] ?? '', | ||
| songCount: json['song_count'] ?? 0, | ||
| length: json['length'] ?? 0, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Avoid empty-string fallback for id in deserialization.
Line 18 allows id == '', but this model’s identity is ID-based (Lines 59-62). Multiple malformed items would become the same entity and break dedupe/vault behavior.
🔧 Proposed fix
factory Genre.fromJson(Map<String, dynamic> json) {
+ final parsedId = (json['id'] ?? json['public_id'])?.toString().trim();
+ if (parsedId == null || parsedId.isEmpty) {
+ throw const FormatException('Genre id is missing');
+ }
+
return Genre(
- id: json['id'] ?? json['public_id'] ?? '',
+ id: parsedId,
name: json['name'] ?? '',
songCount: json['song_count'] ?? 0,
length: json['length'] ?? 0,
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/models/genre.dart` around lines 16 - 23, The deserialization in
Genre.fromJson currently falls back to an empty string for id which can collapse
multiple malformed records into the same identity; change Genre.fromJson to
compute id = json['id'] ?? json['public_id'] and if that value is null or empty
throw a descriptive FormatException (or assert) rather than returning '' so
malformed payloads fail fast; update any calling code/tests if they expect
silent fallbacks.
7e18f21 to
b4955c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/ui/screens/genre_details.dart (1)
204-207: PreferSizedBoxoverContainerfor simple dimension constraints.When only setting height (no decoration, padding, etc.),
SizedBoxis more idiomatic and slightly more performant.♻️ Suggested refactor
- child: Container( - height: 72, - child: const Center(child: Spinner(size: 16)), - ), + child: SizedBox( + height: 72, + child: const Center(child: Spinner(size: 16)), + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genre_details.dart` around lines 204 - 207, Replace the simple Container used solely for height constraints with a SizedBox to be more idiomatic and slightly more performant: locate the Container wrapping Spinner (the widget that currently reads child: Container(height: 72, child: const Center(child: Spinner(size: 16)),) ) and replace it with a SizedBox(height: 72, child: const Center(child: Spinner(size: 16))) ensuring const propagation is preserved where possible.lib/ui/screens/genres.dart (1)
64-64: AddconsttoCupertinoThemeData.The constructor can be made const for a minor performance improvement.
♻️ Suggested refactor
- data: CupertinoThemeData(primaryColor: Colors.white), + data: const CupertinoThemeData(primaryColor: Colors.white),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genres.dart` at line 64, The CupertinoThemeData instance used for the widget's data property should be constructed as a const to improve performance; update the expression that currently reads CupertinoThemeData(primaryColor: Colors.white) to use const CupertinoThemeData(...) instead, ensuring the constructor call in the widget tree (the data: CupertinoThemeData usage) is prefixed with const.test/ui/screens/genres_screen_test.dart (1)
1-66: Good test coverage for basic GenresScreen functionality.The tests effectively cover:
- Rendering genre list with song counts
- Singular/plural handling for "song"
- Unknown genre name fallback
Consider adding tests for error states (
_errored = true) and empty lists in a follow-up, but current coverage is adequate for the happy path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/screens/genres_screen_test.dart` around lines 1 - 66, Tests currently cover happy paths but lack cases for error and empty-list states; add widget tests that set genreProviderMock.genres to [] and to any value while also stubbing genreProviderMock so its errored flag/state (e.g., expose a getter or mock property used by GenresScreen like _errored or an isErrored getter) returns true, then pump the GenresScreen and assert the error UI is shown for the errored case and the empty-state UI/message is shown for the empty-list case; reference GenreProvider, genreProviderMock.genres, genreProviderMock.fetch(), and the GenresScreen widget to locate where to add these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/screens/genre_details.dart`:
- Around line 78-106: The _refresh() method swallows errors and never flips the
_errored flag, which leaves the UI empty with no retry when a refresh fails and
_songs is empty; update the catch block in _refresh to set _errored = true
(inside setState) when an exception occurs and _songs.isEmpty (otherwise leave
_songs as-is), ensure any state changes run only when mounted, and keep the
final block setting _loading = false so the error UI can render and offer retry.
---
Nitpick comments:
In `@lib/ui/screens/genre_details.dart`:
- Around line 204-207: Replace the simple Container used solely for height
constraints with a SizedBox to be more idiomatic and slightly more performant:
locate the Container wrapping Spinner (the widget that currently reads child:
Container(height: 72, child: const Center(child: Spinner(size: 16)),) ) and
replace it with a SizedBox(height: 72, child: const Center(child: Spinner(size:
16))) ensuring const propagation is preserved where possible.
In `@lib/ui/screens/genres.dart`:
- Line 64: The CupertinoThemeData instance used for the widget's data property
should be constructed as a const to improve performance; update the expression
that currently reads CupertinoThemeData(primaryColor: Colors.white) to use const
CupertinoThemeData(...) instead, ensuring the constructor call in the widget
tree (the data: CupertinoThemeData usage) is prefixed with const.
In `@test/ui/screens/genres_screen_test.dart`:
- Around line 1-66: Tests currently cover happy paths but lack cases for error
and empty-list states; add widget tests that set genreProviderMock.genres to []
and to any value while also stubbing genreProviderMock so its errored flag/state
(e.g., expose a getter or mock property used by GenresScreen like _errored or an
isErrored getter) returns true, then pump the GenresScreen and assert the error
UI is shown for the errored case and the empty-state UI/message is shown for the
empty-list case; reference GenreProvider, genreProviderMock.genres,
genreProviderMock.fetch(), and the GenresScreen widget to locate where to add
these tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d662ba71-33ab-41c3-924f-813a4c786244
📒 Files selected for processing (21)
lib/main.dartlib/models/genre.dartlib/models/models.dartlib/providers/genre_provider.dartlib/providers/playable_provider.dartlib/providers/providers.dartlib/router.dartlib/ui/screens/genre_details.dartlib/ui/screens/genres.dartlib/ui/screens/library.dartlib/ui/screens/screens.dartlib/ui/widgets/message_overlay.dartlib/ui/widgets/playable_list_header.darttest/models/genre_test.darttest/providers/download_sync_provider_test.mocks.darttest/providers/recently_played_provider_test.mocks.darttest/ui/screens/genres_screen_test.darttest/ui/screens/genres_screen_test.mocks.darttest/ui/widgets/album_card_test.mocks.darttest/ui/widgets/artist_card_test.mocks.darttest/ui/widgets/song_cache_icon_test.mocks.dart
✅ Files skipped from review due to trivial changes (8)
- lib/providers/providers.dart
- lib/ui/screens/screens.dart
- lib/models/models.dart
- lib/ui/widgets/message_overlay.dart
- test/models/genre_test.dart
- test/ui/screens/genres_screen_test.mocks.dart
- lib/ui/widgets/playable_list_header.dart
- lib/models/genre.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/main.dart
- lib/ui/screens/library.dart
- lib/router.dart
- lib/providers/genre_provider.dart
- Genre model, provider, and API integration (GET /genres, GET /genres/{id}/songs)
- Genres screen with list of all genres and song count
- Genre details screen with sortable/searchable song list
- Genre added to Library menu, router, PlayableListContext enum
- PlayableProvider.fetchForGenre() for fetching songs by genre
phanan
force-pushed
the
feat/genre-support
branch
from
b4955c3 to
2e17c00
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lib/ui/screens/genre_details.dart (2)
129-137:⚠️ Potential issue | 🟠 MajorRender retry/error UI when load fails after initial phase and list is empty.
OopsBoxis only shown during_initialLoading(Line [129]-[137]). If a sort-triggered reload (Line [171]-[179]) fails after_songsis cleared, users get an empty screen without recovery.🔧 Suggested fix
`@override` Widget build(BuildContext context) { if (_initialLoading) { return Scaffold( body: GradientDecoratedContainer( child: _errored ? OopsBox(onRetry: _fetchMore) : const PlayableListScreenPlaceholder(), ), ); } + + if (_errored && _songs.isEmpty && !_loading) { + return Scaffold( + body: GradientDecoratedContainer( + child: OopsBox(onRetry: _fetchMore), + ), + ); + } final displayedSongs = _songs.$sort(_sortConfig).$filter(_searchQuery);Also applies to: 171-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genre_details.dart` around lines 129 - 137, The UI only shows OopsBox while _initialLoading is true, so if a sort-triggered reload clears _songs and then fails users see an empty screen; change the build logic to render OopsBox (with onRetry:_fetchMore) whenever _errored && _songs.isEmpty (not just when _initialLoading), and ensure the sort-reload code that clears _songs sets _errored appropriately and calls setState so the new condition is evaluated (look for the build branch using _initialLoading/_errored/OopsBox and the sort-reload code that clears _songs).
47-76:⚠️ Potential issue | 🟠 MajorProtect
_fetchMore()from stale responses after sort changes.Because Line [48] blocks new fetches while
_loadingis true, a sort change at Line [171]-[179 can leave the old in-flight response to update_songs/_nextPagefor the previous sort mode.🔧 Suggested fix (request versioning + forced reload on sort change)
class _GenreDetailsScreenState extends State<GenreDetailsScreen> { + int _requestVersion = 0; var _songs = <Playable>[]; var _searchQuery = ''; var _loading = false; @@ - Future<void> _fetchMore() async { - if (_loading || _nextPage == null) return; + Future<void> _fetchMore({bool force = false}) async { + if ((!force && _loading) || _nextPage == null) return; + final requestVersion = _requestVersion; - setState(() => _loading = true); + setState(() { + _loading = true; + _errored = false; + }); @@ - if (!mounted) return; + if (!mounted || requestVersion != _requestVersion) return; setState(() { _songs = [..._songs, ...result.items].toSet().toList(); _nextPage = result.nextPage; _initialLoading = false; + _errored = false; @@ } catch (_) { - if (mounted) setState(() => _errored = true); + if (mounted && requestVersion == _requestVersion) { + setState(() => _errored = true); + } } finally { - if (mounted) setState(() => _loading = false); + if (mounted && requestVersion == _requestVersion) { + setState(() => _loading = false); + } } } @@ onMenuItemSelected: (config) { AppState.set('genre.sort', config); setState(() { + _requestVersion++; _sortConfig = config; _songs.clear(); _nextPage = 1; + _errored = false; }); - _fetchMore(); + _fetchMore(force: true); },Also applies to: 171-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/genre_details.dart` around lines 47 - 76, The _fetchMore() routine can apply stale in-flight responses after a sort change because _loading blocks new fetches; add a request-version token (e.g., an int _fetchVersion) that is incremented whenever the sort is changed (the handler at the sort-change block referenced around lines 171-179), capture the current version at the start of _fetchMore(), and after awaiting paginateByGenre compare the captured version to the current _fetchVersion and abort applying results if they differ; also ensure the sort-change handler increments _fetchVersion and resets state (_songs, _nextPage, _initialLoading, _cover) before starting a fresh fetch so only responses matching the current version update _songs/_nextPage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/ui/screens/genre_details.dart`:
- Around line 129-137: The UI only shows OopsBox while _initialLoading is true,
so if a sort-triggered reload clears _songs and then fails users see an empty
screen; change the build logic to render OopsBox (with onRetry:_fetchMore)
whenever _errored && _songs.isEmpty (not just when _initialLoading), and ensure
the sort-reload code that clears _songs sets _errored appropriately and calls
setState so the new condition is evaluated (look for the build branch using
_initialLoading/_errored/OopsBox and the sort-reload code that clears _songs).
- Around line 47-76: The _fetchMore() routine can apply stale in-flight
responses after a sort change because _loading blocks new fetches; add a
request-version token (e.g., an int _fetchVersion) that is incremented whenever
the sort is changed (the handler at the sort-change block referenced around
lines 171-179), capture the current version at the start of _fetchMore(), and
after awaiting paginateByGenre compare the captured version to the current
_fetchVersion and abort applying results if they differ; also ensure the
sort-change handler increments _fetchVersion and resets state (_songs,
_nextPage, _initialLoading, _cover) before starting a fresh fetch so only
responses matching the current version update _songs/_nextPage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c39dfa2-2741-48cd-b765-2df2024f0aad
📒 Files selected for processing (21)
lib/main.dartlib/models/genre.dartlib/models/models.dartlib/providers/genre_provider.dartlib/providers/playable_provider.dartlib/providers/providers.dartlib/router.dartlib/ui/screens/genre_details.dartlib/ui/screens/genres.dartlib/ui/screens/library.dartlib/ui/screens/screens.dartlib/ui/widgets/message_overlay.dartlib/ui/widgets/playable_list_header.darttest/models/genre_test.darttest/providers/download_sync_provider_test.mocks.darttest/providers/recently_played_provider_test.mocks.darttest/ui/screens/genres_screen_test.darttest/ui/screens/genres_screen_test.mocks.darttest/ui/widgets/album_card_test.mocks.darttest/ui/widgets/artist_card_test.mocks.darttest/ui/widgets/song_cache_icon_test.mocks.dart
✅ Files skipped from review due to trivial changes (9)
- lib/main.dart
- lib/models/models.dart
- lib/ui/screens/screens.dart
- lib/ui/widgets/message_overlay.dart
- lib/ui/widgets/playable_list_header.dart
- test/ui/screens/genres_screen_test.mocks.dart
- test/models/genre_test.dart
- test/ui/screens/genres_screen_test.dart
- lib/models/genre.dart
🚧 Files skipped from review as they are similar to previous changes (8)
- lib/providers/providers.dart
- lib/ui/screens/library.dart
- lib/router.dart
- test/ui/widgets/album_card_test.mocks.dart
- test/ui/widgets/artist_card_test.mocks.dart
- test/providers/download_sync_provider_test.mocks.dart
- lib/ui/screens/genres.dart
- lib/providers/genre_provider.dart
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Genremodel withid,name,songCount,lengthfieldsGenreProviderto fetch genres fromGET /api/genresGenresScreenlisting all genres with song countsGenreDetailsScreenwith paginated song list (GET /api/genres/{id}/songs), sorting, search, alphabet scrollbar, and cover image stackpaginateByGenre()toPlayableProviderwith scroll-to-load paginationgenretoPlayableListContextenumTest plan
Summary by CodeRabbit
New Features
Style
Tests