Add radio station support#138
Merged
Merged
Conversation
|
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 Radio feature: a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as RadioStationsScreen
participant StationProv as RadioStationProvider
participant API as Backend API
participant Notifier as ChangeNotifier
rect rgba(100,150,200,0.5)
UI->>StationProv: fetchAll()
StationProv->>API: GET /radio/stations
API-->>StationProv: [stations JSON]
StationProv->>StationProv: parse & set _stations
StationProv->>Notifier: notifyListeners()
Notifier-->>UI: rebuild with stations
end
sequenceDiagram
participant UI as Radio UI (button)
participant PlayerProv as RadioPlayerProvider
participant Prefs as Preferences
participant Audio as AudioPlayer / AudioHandler
participant API as Backend API
rect rgba(150,100,200,0.5)
UI->>PlayerProv: play(station)
PlayerProv->>Prefs: read host & audioToken
alt need stream URL resolution
PlayerProv->>API: optional resolve stream URL
API-->>PlayerProv: stream_url
end
PlayerProv->>Audio: _player.setUrl(stream_url)
PlayerProv->>Audio: _player.play()
Audio-->>PlayerProv: playing/processing events
PlayerProv->>PlayerProv: update playing/loading/streamTitle
PlayerProv->>PlayerProv: notifyListeners()
PlayerProv-->>UI: mini-player updates
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
Comment |
![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 (3)
lib/models/radio_station.dart (1)
20-28: HardenfromJsonrequired-field parsing.Line 22–24 currently trust dynamic payload types. Add explicit type checks so malformed API payloads fail fast with a clear error.
Proposed refactor
factory RadioStation.fromJson(Map<String, dynamic> json) { + final id = json['id']; + final name = json['name']; + final url = json['url']; + if (id is! String || name is! String || url is! String) { + throw const FormatException('Invalid radio station payload'); + } + return RadioStation( - id: json['id'], - name: json['name'], - url: json['url'], + id: id, + name: name, + url: url, logo: json['logo'], description: json['description'], - isPublic: json['is_public'] ?? false, + isPublic: json['is_public'] is bool ? json['is_public'] as bool : false, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/models/radio_station.dart` around lines 20 - 28, RadioStation.fromJson currently assumes keys exist and have correct types; update it to validate required fields (id, name, url) and optional fields (logo, description, is_public) by checking types (e.g., id is int or String as your model expects, name/url are String, is_public is bool) and throwing a clear FormatException when a field is missing or of the wrong type; reference the RadioStation.fromJson constructor and the field names id, name, url, logo, description, isPublic in your checks and include the offending key and expected type in the exception message so malformed API payloads fail fast with a clear error.test/models/radio_station_test.dart (1)
5-42: Add a missing-key fallback test foris_public.You verify
true/false, but not the “key absent → false” path from Line 27 in the model.Suggested test case
group('RadioStation.fromJson', () { + test('defaults isPublic to false when is_public is missing', () { + final station = RadioStation.fromJson({ + 'id': 'station-3', + 'name': 'Talk Radio', + 'url': 'https://stream.talk.example/live', + }); + + expect(station.isPublic, isFalse); + }); + test('parses all fields', () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/radio_station_test.dart` around lines 5 - 42, Add a test to cover the "missing is_public key → false" fallback in RadioStation.fromJson: create a new test case in test/models/radio_station_test.dart (within the RadioStation.fromJson group) that builds a JSON without the 'is_public' key, calls RadioStation.fromJson(json), and asserts that the resulting station.isPublic is false; reference the model's fromJson handling of 'is_public' (the logic around Line 27) to ensure this absent-key path is exercised.lib/ui/screens/radio_stations.dart (1)
51-52: Sort a local copy instead of the provider-returned list.This mutates whatever
provider.stationsreturns on every rebuild. If that getter exposes the backing list,build()is mutating provider state; if it later returns an unmodifiable view, this starts throwing. Copy first and sort the copy.💡 Minimal fix
- final stations = provider.stations - ..sort((a, b) => a.name.compareTo(b.name)); + final stations = [...provider.stations] + ..sort((a, b) => a.name.compareTo(b.name));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 51 - 52, The current code mutates provider.stations by calling sort on it inside build() (final stations = provider.stations ..sort(...)), which can modify provider state or break if the getter returns an unmodifiable view; fix by creating a local mutable copy before sorting (e.g., assign a new List from provider.stations) and call sort on that copy so provider.stations remains untouched (update the stations variable initialization used in the build method accordingly).
🤖 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/radio_station_provider.dart`:
- Around line 65-68: The remove(RadioStation station) function currently
fire-and-forgets the HTTP delete call (delete('radio/stations/${station.id}'))
then immediately mutates local state (_stations.remove(station);
notifyListeners()), causing possible UI/server divergence; change it to await
the delete call and only remove from _stations and call notifyListeners() after
a successful response, wrapping the await in try/catch to handle and surface
errors (log/rethrow or show user feedback) so failures do not remove the station
locally.
- Line 10: The stations getter currently returns the mutable backing list
_stations, allowing external mutation without notifications; change the stations
getter (in the provider class that defines List<RadioStation> get stations =>
_stations) to return an unmodifiable view or a defensive copy (e.g.,
List.unmodifiable(_stations) or _stations.toList()) so callers cannot mutate the
provider's internal _stations directly and notifications remain reliable.
In `@lib/ui/screens/radio_stations.dart`:
- Around line 31-40: The _fetchData method is swallowing all exceptions and
allowing failures to present as an empty list; update _fetchData to set an error
state instead of ignoring the catch: add a new state field (e.g. _error or
_loadError), in the catch block assign the caught exception or a user-friendly
message to that field and optionally log it, then ensure setState is called so
the UI can react; keep the finally block to clear _loading when mounted. Also
update the UI that reads RadioStationProvider.fetchAll() and the "No radio
stations" path to show the error state (and a retry action) when
_error/_loadError is set. Use the existing symbols _fetchData, _loading,
RadioStationProvider.fetchAll, mounted and setState to locate and modify the
code.
- Around line 154-161: The _playStation method currently instantiates a new
AudioPlayer per tap which leaks native resources; instead add a single
AudioPlayer field on the State (e.g., _audioPlayer), initialize it in initState
(or lazily on first use), reuse it in _playStation by calling
_audioPlayer.setUrl(streamUrl) and _audioPlayer.play(), and ensure you call
_audioPlayer.dispose() in the widget's dispose() to release native resources;
update any references to the local player variable accordingly (AudioPlayer,
_playStation, initState, dispose).
- Around line 180-252: The dialog uses the builder's local context for
post-dialog feedback and always pops the dialog on both success and failure;
change it to call showOverlay with the screen-level context (not the
builder/context parameter) after verifying mounted, and only pop the dialog on
success: in the CupertinoAlertDialog builder where you call provider.create (the
create call inside the onPressed of the 'Add' CupertinoDialogAction), replace
Navigator.pop(context); showOverlay(context, ...) with logic that (1) on success
calls Navigator.pop(...) and then uses the outer screen context (and a mounted
check) to call showOverlay(...), and (2) on error does not call
Navigator.pop(...) so the edit dialog stays open and instead shows an
inline/error overlay using the screen context so the user can correct and retry.
---
Nitpick comments:
In `@lib/models/radio_station.dart`:
- Around line 20-28: RadioStation.fromJson currently assumes keys exist and have
correct types; update it to validate required fields (id, name, url) and
optional fields (logo, description, is_public) by checking types (e.g., id is
int or String as your model expects, name/url are String, is_public is bool) and
throwing a clear FormatException when a field is missing or of the wrong type;
reference the RadioStation.fromJson constructor and the field names id, name,
url, logo, description, isPublic in your checks and include the offending key
and expected type in the exception message so malformed API payloads fail fast
with a clear error.
In `@lib/ui/screens/radio_stations.dart`:
- Around line 51-52: The current code mutates provider.stations by calling sort
on it inside build() (final stations = provider.stations ..sort(...)), which can
modify provider state or break if the getter returns an unmodifiable view; fix
by creating a local mutable copy before sorting (e.g., assign a new List from
provider.stations) and call sort on that copy so provider.stations remains
untouched (update the stations variable initialization used in the build method
accordingly).
In `@test/models/radio_station_test.dart`:
- Around line 5-42: Add a test to cover the "missing is_public key → false"
fallback in RadioStation.fromJson: create a new test case in
test/models/radio_station_test.dart (within the RadioStation.fromJson group)
that builds a JSON without the 'is_public' key, calls
RadioStation.fromJson(json), and asserts that the resulting station.isPublic is
false; reference the model's fromJson handling of 'is_public' (the logic around
Line 27) to ensure this absent-key path is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21dfa38d-c823-4ea9-8d21-4b956ad6d9b4
📒 Files selected for processing (9)
lib/main.dartlib/models/models.dartlib/models/radio_station.dartlib/providers/providers.dartlib/providers/radio_station_provider.dartlib/ui/screens/library.dartlib/ui/screens/radio_stations.dartlib/ui/screens/screens.darttest/models/radio_station_test.dart
Comment on lines
+65
to
+68
| Future<void> remove(RadioStation station) async { | ||
| delete('radio/stations/${station.id}'); | ||
| _stations.remove(station); | ||
| notifyListeners(); |
There was a problem hiding this comment.
Await delete before mutating local state.
Line 66 currently fire-and-forgets the server delete, then removes locally. If the request fails, UI and server diverge.
Proposed fix
Future<void> remove(RadioStation station) async {
- delete('radio/stations/${station.id}');
- _stations.remove(station);
+ await delete('radio/stations/${station.id}');
+ _stations.removeWhere((s) => s.id == station.id);
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
| Future<void> remove(RadioStation station) async { | |
| delete('radio/stations/${station.id}'); | |
| _stations.remove(station); | |
| notifyListeners(); | |
| Future<void> remove(RadioStation station) async { | |
| await delete('radio/stations/${station.id}'); | |
| _stations.removeWhere((s) => s.id == station.id); | |
| notifyListeners(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/providers/radio_station_provider.dart` around lines 65 - 68, The
remove(RadioStation station) function currently fire-and-forgets the HTTP delete
call (delete('radio/stations/${station.id}')) then immediately mutates local
state (_stations.remove(station); notifyListeners()), causing possible UI/server
divergence; change it to await the delete call and only remove from _stations
and call notifyListeners() after a successful response, wrapping the await in
try/catch to handle and surface errors (log/rethrow or show user feedback) so
failures do not remove the station locally.
Comment on lines
+31
to
+40
| Future<void> _fetchData() async { | ||
| if (_loading) return; | ||
| setState(() => _loading = true); | ||
|
|
||
| try { | ||
| await context.read<RadioStationProvider>().fetchAll(); | ||
| } catch (_) { | ||
| } finally { | ||
| if (mounted) setState(() => _loading = false); | ||
| } |
There was a problem hiding this comment.
Don't collapse fetch failures into the empty state.
_fetchData() swallows every exception, so a failed initial load/refresh can fall straight through to the "No radio stations" UI. Please track an error state or surface the failure instead of telling the user their library is empty.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/screens/radio_stations.dart` around lines 31 - 40, The _fetchData
method is swallowing all exceptions and allowing failures to present as an empty
list; update _fetchData to set an error state instead of ignoring the catch: add
a new state field (e.g. _error or _loadError), in the catch block assign the
caught exception or a user-friendly message to that field and optionally log it,
then ensure setState is called so the UI can react; keep the finally block to
clear _loading when mounted. Also update the UI that reads
RadioStationProvider.fetchAll() and the "No radio stations" path to show the
error state (and a retry action) when _error/_loadError is set. Use the existing
symbols _fetchData, _loading, RadioStationProvider.fetchAll, mounted and
setState to locate and modify the code.
Comment on lines
+154
to
+161
| Future<void> _playStation(RadioStation station) async { | ||
| final streamUrl = | ||
| '${preferences.host}/radio/stream/${station.id}?t=${preferences.audioToken}'; | ||
|
|
||
| try { | ||
| final player = AudioPlayer(); | ||
| await player.setUrl(streamUrl); | ||
| await player.play(); |
There was a problem hiding this comment.
Reuse one AudioPlayer and dispose it in dispose().
A new player is created for every tap and never closed. Repeated plays can leak native resources and leave previous radio streams running. Keep the player on the state object and switch URLs on that single instance.
💡 Minimal fix
class _RadioStationsScreenState extends State<RadioStationsScreen> {
var _loading = false;
+ final AudioPlayer _player = AudioPlayer();
+
+ `@override`
+ void dispose() {
+ _player.dispose();
+ super.dispose();
+ }
`@override`
void initState() {
@@
Future<void> _playStation(RadioStation station) async {
@@
try {
- final player = AudioPlayer();
- await player.setUrl(streamUrl);
- await player.play();
+ await _player.setUrl(streamUrl);
+ await _player.play();
} catch (e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/screens/radio_stations.dart` around lines 154 - 161, The _playStation
method currently instantiates a new AudioPlayer per tap which leaks native
resources; instead add a single AudioPlayer field on the State (e.g.,
_audioPlayer), initialize it in initState (or lazily on first use), reuse it in
_playStation by calling _audioPlayer.setUrl(streamUrl) and _audioPlayer.play(),
and ensure you call _audioPlayer.dispose() in the widget's dispose() to release
native resources; update any references to the local player variable accordingly
(AudioPlayer, _playStation, initState, dispose).
Comment on lines
+180
to
+252
| await showCupertinoDialog( | ||
| context: context, | ||
| builder: (context) => CupertinoAlertDialog( | ||
| title: const Text('Add Radio Station'), | ||
| content: Column( | ||
| children: [ | ||
| const SizedBox(height: 12), | ||
| CupertinoTextField( | ||
| controller: nameController, | ||
| placeholder: 'Station Name', | ||
| autofocus: true, | ||
| decoration: BoxDecoration( | ||
| color: CupertinoColors.tertiarySystemFill, | ||
| borderRadius: BorderRadius.circular(8), | ||
| ), | ||
| padding: | ||
| const EdgeInsets.symmetric(horizontal: 12, vertical: 10), | ||
| ), | ||
| const SizedBox(height: 8), | ||
| CupertinoTextField( | ||
| controller: urlController, | ||
| placeholder: 'Stream URL', | ||
| keyboardType: TextInputType.url, | ||
| decoration: BoxDecoration( | ||
| color: CupertinoColors.tertiarySystemFill, | ||
| borderRadius: BorderRadius.circular(8), | ||
| ), | ||
| padding: | ||
| const EdgeInsets.symmetric(horizontal: 12, vertical: 10), | ||
| ), | ||
| const SizedBox(height: 8), | ||
| CupertinoTextField( | ||
| controller: descController, | ||
| placeholder: 'Description (optional)', | ||
| maxLines: 2, | ||
| decoration: BoxDecoration( | ||
| color: CupertinoColors.tertiarySystemFill, | ||
| borderRadius: BorderRadius.circular(8), | ||
| ), | ||
| padding: | ||
| const EdgeInsets.symmetric(horizontal: 12, vertical: 10), | ||
| ), | ||
| ], | ||
| ), | ||
| actions: [ | ||
| CupertinoDialogAction( | ||
| child: const Text('Cancel'), | ||
| onPressed: () => Navigator.pop(context), | ||
| ), | ||
| CupertinoDialogAction( | ||
| isDefaultAction: true, | ||
| child: const Text('Add'), | ||
| onPressed: () async { | ||
| final name = nameController.text.trim(); | ||
| final url = urlController.text.trim(); | ||
| if (name.isEmpty || url.isEmpty) return; | ||
|
|
||
| try { | ||
| await provider.create( | ||
| name: name, | ||
| url: url, | ||
| description: descController.text.trim(), | ||
| ); | ||
| Navigator.pop(context); | ||
| showOverlay(context, caption: 'Station added'); | ||
| } catch (_) { | ||
| Navigator.pop(context); | ||
| showOverlay( | ||
| context, | ||
| caption: 'Error', | ||
| icon: CupertinoIcons.exclamationmark_triangle, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and check its size
wc -l lib/ui/screens/radio_stations.dartRepository: koel/player
Length of output: 93
🏁 Script executed:
# Read lines 180-252 to verify the code structure
sed -n '180,252p' lib/ui/screens/radio_stations.dartRepository: koel/player
Length of output: 2742
🏁 Script executed:
# Read lines 315-383 to verify the pattern applies there
sed -n '315,383p' lib/ui/screens/radio_stations.dartRepository: koel/player
Length of output: 2597
🏁 Script executed:
# Check the class definition and imports
head -30 lib/ui/screens/radio_stations.dart && echo "---" && sed -n '1,50p' lib/ui/screens/radio_stations.dart | grep -E "^(class|import|extends)"Repository: koel/player
Length of output: 1625
🏁 Script executed:
# Find showOverlay function definition
rg "showOverlay" lib/ui/screens/radio_stations.dart -B 2 -A 5Repository: koel/player
Length of output: 901
🏁 Script executed:
# Search for showOverlay definition in the codebase
rg "void showOverlay|Future.*showOverlay" -t dartRepository: koel/player
Length of output: 108
🏁 Script executed:
# Get more context around the dialog methods
sed -n '170,260p' lib/ui/screens/radio_stations.dartRepository: koel/player
Length of output: 3104
🏁 Script executed:
# Check the showOverlay implementation
cat lib/ui/widgets/message_overlay.dartRepository: koel/player
Length of output: 4410
🏁 Script executed:
# Also check the edit dialog full context
sed -n '305,390p' lib/ui/screens/radio_stations.dartRepository: koel/player
Length of output: 3040
🏁 Script executed:
# Let's check how showOverlay is used elsewhere in the app to see best practices
rg "showOverlay" lib/ui/screens/ -B 2 -A 1 | head -40Repository: koel/player
Length of output: 2621
🏁 Script executed:
# Check if there are other instances of the mounted check pattern used in this file
rg "if.*mounted" lib/ui/screens/radio_stations.dart -B 1 -A 2Repository: koel/player
Length of output: 215
🏁 Script executed:
# Check more context around the existing mounted usage to understand the pattern
sed -n '125,145p' lib/ui/screens/radio_stations.dartRepository: koel/player
Length of output: 873
Use screen context for feedback after dialog close, and keep edit dialog open on save failure.
The success path pops the dialog and immediately calls showOverlay(context, ...) using the now-invalid dialog builder context. After Navigator.pop(), that context is no longer valid for overlay operations. Use the screen context with a mounted check instead. Additionally, the edit dialog's failure path pops the dialog without showing feedback, discarding the user's input and leaving no retry option.
Pattern for both dialogs
- builder: (context) => CupertinoAlertDialog(
+ builder: (dialogContext) => CupertinoAlertDialog(
@@
try {
await provider.create(
name: name,
url: url,
description: descController.text.trim(),
);
- Navigator.pop(context);
- showOverlay(context, caption: 'Station added');
+ Navigator.pop(dialogContext);
+ if (!mounted) return;
+ showOverlay(this.context, caption: 'Station added');
} catch (_) {
- Navigator.pop(context);
+ if (!mounted) return;
showOverlay(
- context,
+ this.context,
caption: 'Error',
icon: CupertinoIcons.exclamationmark_triangle,
);
}For the edit dialog, apply the same context fix, but do not pop on failure to let the user correct and retry:
try {
await provider.update(
station,
name: name,
url: url,
description: descController.text.trim(),
);
- Navigator.pop(context);
- showOverlay(context, caption: 'Station updated');
+ Navigator.pop(dialogContext);
+ if (!mounted) return;
+ showOverlay(this.context, caption: 'Station updated');
} catch (_) {
- Navigator.pop(context);
+ if (!mounted) return;
+ showOverlay(
+ this.context,
+ caption: 'Error',
+ icon: CupertinoIcons.exclamationmark_triangle,
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/screens/radio_stations.dart` around lines 180 - 252, The dialog uses
the builder's local context for post-dialog feedback and always pops the dialog
on both success and failure; change it to call showOverlay with the screen-level
context (not the builder/context parameter) after verifying mounted, and only
pop the dialog on success: in the CupertinoAlertDialog builder where you call
provider.create (the create call inside the onPressed of the 'Add'
CupertinoDialogAction), replace Navigator.pop(context); showOverlay(context,
...) with logic that (1) on success calls Navigator.pop(...) and then uses the
outer screen context (and a mounted check) to call showOverlay(...), and (2) on
error does not call Navigator.pop(...) so the edit dialog stays open and instead
shows an inline/error overlay using the screen context so the user can correct
and retry.
phanan
force-pushed
the
feat/radio-stations
branch
from
78c21df to
63fc47d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/providers/radio_player_provider.dart (1)
39-62: Consider stopping previous playback before starting new station.If
play()is called while already loading/playing another station, the previous stream setup may overlap. Calling_player.stop()first ensures clean state.Proposed fix
Future<void> play(RadioStation station) async { // Pause the main queue player when radio starts if (audioHandler.playbackState.value.playing) { await audioHandler.pause(); } + // Stop any current radio playback first + await _player.stop(); + _currentStation = station; _streamTitle = null; _loading = true; notifyListeners();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/radio_player_provider.dart` around lines 39 - 62, The play(RadioStation) method can overlap previous streams; before setting a new URL call and await _player.stop() to ensure the previous playback is fully stopped (do this after pausing the main queue via audioHandler.pause()), then proceed to setUrl and play; keep the existing state updates (_currentStation, _streamTitle, _loading, notifyListeners) and error handling, but ensure you stop the _player first to prevent overlapping stream setups.lib/ui/widgets/mini_player.dart (1)
121-122: Consider null-safe access instead of force-unwrap.While
activebeing true impliescurrentStation != null, the force-unwrap (!) couples this assumption. A defensive approach would usecurrentStationas a parameter or add a null check.Alternative: pass station as parameter
- Widget _buildRadioMiniPlayer(RadioPlayerProvider radioPlayer) { - final station = radioPlayer.currentStation!; + Widget _buildRadioMiniPlayer(RadioPlayerProvider radioPlayer, RadioStation station) {Then in
build():if (radioPlayer.active) { - return _buildRadioMiniPlayer(radioPlayer); + return _buildRadioMiniPlayer(radioPlayer, radioPlayer.currentStation!); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/mini_player.dart` around lines 121 - 122, The _buildRadioMiniPlayer currently force-unwraps radioPlayer.currentStation with `!`; change this to a null-safe approach by either (A) adding a null-check inside _buildRadioMiniPlayer (e.g., return a SizedBox/empty widget or handle the null case when radioPlayer.currentStation == null) or (B) change _buildRadioMiniPlayer signature to accept a non-null Station parameter and pass radioPlayer.currentStation from build() only when active is true (so callers guarantee non-null); update all call sites in build() to pass the station if you choose option B.
🤖 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/radio_player_provider.dart`:
- Around line 73-79: togglePlayPause may call _player.play() when there is no
active station URL, causing a silent failure; update the togglePlayPause method
to check that _currentStation (or its URL) is non-null and valid before calling
_player.play(), short-circuiting (or logging/throwing) when no station is set,
while still allowing _player.pause() when currently playing; use the symbols
togglePlayPause, _currentStation, _player.play(), _player.pause(), and _playing
to locate and implement the guard.
In `@lib/ui/screens/radio_stations.dart`:
- Around line 49-50: The current assignment using `final stations =
provider.stations..sort(...)` mutates the provider's backing list on every
build; instead create a non-mutating sorted copy (e.g., call
`toList()`/`List.from()` on `provider.stations`) and sort that copy before use
so the provider's internal list is not changed and no unintended side effects
occur; update the code where `stations` is assigned (referencing
`provider.stations` and the `stations` local variable) to construct and sort a
copy rather than using the cascade `..sort`.
In `@lib/ui/widgets/mini_player.dart`:
- Around line 44-51: The stream listener in mini_player.dart calls
context.read<RadioPlayerProvider>() inside an async/stream callback which can
run after the widget is disposed, causing use-after-dispose; fix by either (A)
moving the radio-stop responsibility into RadioPlayerProvider.play() so
RadioPlayerProvider decides to stop the radio when queue playback starts (update
RadioPlayerProvider.play() to call stop() on its active radio internally), or
(B) if keeping the logic in the widget, capture the provider reference once
(final radioPlayer = context.read<RadioPlayerProvider>() in initState) and/or
guard the callback with if (!mounted) return before calling radioPlayer.stop(),
and ensure the stream subscription is cancelled in dispose to prevent callbacks
after tear-down.
---
Nitpick comments:
In `@lib/providers/radio_player_provider.dart`:
- Around line 39-62: The play(RadioStation) method can overlap previous streams;
before setting a new URL call and await _player.stop() to ensure the previous
playback is fully stopped (do this after pausing the main queue via
audioHandler.pause()), then proceed to setUrl and play; keep the existing state
updates (_currentStation, _streamTitle, _loading, notifyListeners) and error
handling, but ensure you stop the _player first to prevent overlapping stream
setups.
In `@lib/ui/widgets/mini_player.dart`:
- Around line 121-122: The _buildRadioMiniPlayer currently force-unwraps
radioPlayer.currentStation with `!`; change this to a null-safe approach by
either (A) adding a null-check inside _buildRadioMiniPlayer (e.g., return a
SizedBox/empty widget or handle the null case when radioPlayer.currentStation ==
null) or (B) change _buildRadioMiniPlayer signature to accept a non-null Station
parameter and pass radioPlayer.currentStation from build() only when active is
true (so callers guarantee non-null); update all call sites in build() to pass
the station if you choose option B.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66a76f97-e342-4e02-8a60-c333721b51e7
📒 Files selected for processing (12)
lib/main.dartlib/models/models.dartlib/models/radio_station.dartlib/providers/providers.dartlib/providers/radio_player_provider.dartlib/providers/radio_station_provider.dartlib/ui/screens/library.dartlib/ui/screens/radio_stations.dartlib/ui/screens/screens.dartlib/ui/widgets/mini_player.darttest/models/radio_station_test.darttest/providers/radio_player_provider_test.dart
✅ Files skipped from review due to trivial changes (6)
- lib/models/models.dart
- lib/ui/screens/screens.dart
- lib/providers/providers.dart
- test/providers/radio_player_provider_test.dart
- test/models/radio_station_test.dart
- lib/models/radio_station.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/main.dart
- lib/ui/screens/library.dart
Comment on lines
+49
to
+50
| final stations = provider.stations | ||
| ..sort((a, b) => a.name.compareTo(b.name)); |
There was a problem hiding this comment.
In-place sort mutates provider's backing list on every build.
The cascade provider.stations..sort(...) modifies the provider's internal list without notifyListeners(). Create a sorted copy instead to avoid side effects.
Proposed fix
builder: (context, provider, navigationBar) {
- final stations = provider.stations
- ..sort((a, b) => a.name.compareTo(b.name));
+ final stations = [...provider.stations]
+ ..sort((a, b) => a.name.compareTo(b.name));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/screens/radio_stations.dart` around lines 49 - 50, The current
assignment using `final stations = provider.stations..sort(...)` mutates the
provider's backing list on every build; instead create a non-mutating sorted
copy (e.g., call `toList()`/`List.from()` on `provider.stations`) and sort that
copy before use so the provider's internal list is not changed and no unintended
side effects occur; update the code where `stations` is assigned (referencing
`provider.stations` and the `stations` local variable) to construct and sort a
copy rather than using the cascade `..sort`.
63fc47d to
d99add5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
lib/ui/screens/radio_stations.dart (3)
49-50:⚠️ Potential issue | 🟠 MajorSort a copy, not
provider.stations.This mutates provider state during build. Once the getter is made immutable, this path will also start throwing.
Minimal fix
- final stations = provider.stations + final stations = [...provider.stations] ..sort((a, b) => a.name.compareTo(b.name));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 49 - 50, The current code mutates provider.stations by calling sort on it during build; instead create a copy and sort that copy (e.g., use toList() or List.from(provider.stations) into the local variable `stations`) so you don't modify the provider state; update the `stations` assignment before the sort to operate on the copied list used in the widget tree.
29-38:⚠️ Potential issue | 🟠 MajorDo not turn load failures into an empty library.
Swallowing every exception here makes a broken fetch look identical to “No radio stations.” Keep an error state and retry affordance instead of falling through to the empty-state UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 29 - 38, The _fetchData method currently swallows all exceptions making failures indistinguishable from an empty list; modify _fetchData to preserve and surface errors by adding a local state field (e.g., _loadError or _errorMessage) and set it in the catch block with the caught exception or a user-friendly message, clear that error on a successful fetch (before or after calling context.read<RadioStationProvider>().fetchAll()), and leave the finally block to only reset _loading; also add a retry affordance in the UI that calls _fetchData so users can retry failed loads. Ensure references to _fetchData and context.read<RadioStationProvider>().fetchAll() are updated to use the new error state for rendering an error view instead of the empty-state UI.
173-245:⚠️ Potential issue | 🟠 MajorUse the screen context for overlays, and keep the dialog open on save failure.
After
Navigator.pop(context), the dialog builder context is no longer safe forshowOverlay(...). Both handlers also close the form on failure, and the edit path drops the error silently. Pop with the dialog context, then usethis.contextafter amountedcheck for feedback.Pattern to apply to both dialogs
- builder: (context) => CupertinoAlertDialog( + builder: (dialogContext) => CupertinoAlertDialog( @@ - Navigator.pop(context); - showOverlay(context, caption: 'Station added'); + Navigator.pop(dialogContext); + if (!mounted) return; + showOverlay(this.context, caption: 'Station added'); } catch (_) { - Navigator.pop(context); - showOverlay( - context, + if (!mounted) return; + showOverlay( + this.context, caption: 'Error', icon: CupertinoIcons.exclamationmark_triangle, ); }Apply the same pattern to the edit dialog, but keep it open on failure so the user can correct and retry.
Also applies to: 308-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 173 - 245, The dialog currently pops using the dialog builder context and then immediately calls showOverlay with that same (now invalid) context and also closes the form on save failure; change the flow so you call Navigator.pop(dialogContext) where dialogContext is the builder-provided context when you need to close the dialog, then after awaiting provider.create(...) and verifying mounted use the screen's context (this.context) to call showOverlay; in the catch block do not pop the dialog for the edit dialog path (keep it open so the user can fix errors), instead show the error overlay using this.context after a mounted check, and only pop the dialog on successful create/edit (reference the provider.create call, showOverlay calls, the dialog builder context variable, Navigator.pop, and mounted).lib/providers/radio_station_provider.dart (2)
65-68:⚠️ Potential issue | 🔴 CriticalAwait the DELETE before mutating local state.
This currently fire-and-forgets the backend delete and removes by object identity. A failed request leaves UI/server out of sync, and
_stations.remove(station)can miss if the instance changes.Minimal fix
Future<void> remove(RadioStation station) async { - delete('radio/stations/${station.id}'); - _stations.remove(station); + await delete('radio/stations/${station.id}'); + _stations.removeWhere((s) => s.id == station.id); notifyListeners(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/radio_station_provider.dart` around lines 65 - 68, In remove(RadioStation station) await the async delete('radio/stations/${station.id}') call and verify success before mutating local state; if the request fails, surface/log the error and do not call notifyListeners. When removing from _stations, remove by matching id (e.g. find and remove where s.id == station.id) instead of _stations.remove(station) to avoid identity mismatches. Ensure notifyListeners() is only called after successful deletion and state mutation.
10-10:⚠️ Potential issue | 🟠 MajorDo not expose
_stationsdirectly.Returning the backing list lets callers mutate provider state without notifications. The new screen already does this via
..sort(...), so the provider can change during build with nonotifyListeners().Minimal fix
+import 'dart:collection'; import 'package:app/mixins/stream_subscriber.dart'; @@ - List<RadioStation> get stations => _stations; + UnmodifiableListView<RadioStation> get stations => + UnmodifiableListView(_stations);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/radio_station_provider.dart` at line 10, The public getter stations currently returns the backing list _stations allowing external code (e.g., callers using ..sort(...)) to mutate provider state without notifyListeners; change the getter implementation in the provider (the stations getter on the class that holds _stations) to return an immutable or defensive copy (for example a List.unmodifiable(_stations) or a new list [..._stations]) so callers cannot modify the internal list directly, and ensure any sorting/mutations are performed through provider methods that call notifyListeners() (e.g., expose a sortStations or updateStations method) instead of modifying the returned list.
🤖 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/radio_station_provider.dart`:
- Around line 13-16: The logout listener currently clears _stations but doesn't
prevent an in-flight fetchAll() from writing results afterward; add a simple
session/request token (e.g. an int _fetchToken) on the provider, increment it
inside the AuthProvider.userLoggedOutStream.listen handler (where you call
_stations.clear(); notifyListeners();), and in fetchAll() capture the current
token at start and before mutating _stations check that the token still matches
— if it doesn't, drop the response. This keeps the
subscribe(AuthProvider.userLoggedOutStream.listen(...)) logic and
notifyListeners() intact while ensuring late fetchAll() responses are ignored
and cannot repopulate stations after logout.
- Around line 12-17: RadioStationProvider subscribes to
AuthProvider.userLoggedOutStream in the constructor but never cancels it; add a
private StreamSubscription field (e.g. _logoutSub) to store the result of
subscribe(AuthProvider.userLoggedOutStream.listen(...)) inside the
RadioStationProvider constructor, then override dispose() in
RadioStationProvider to call await or _logoutSub?.cancel() (and set it to null)
before calling super.dispose(), ensuring the subscription is cancelled so
_stations.clear() and notifyListeners() won't run after disposal.
In `@lib/ui/screens/radio_stations.dart`:
- Around line 107-113: The dismissal currently calls provider.remove(...) in
onDismissed which lets the row animate away before the async API call completes;
move the deletion into confirmDismiss by calling and awaiting
provider.remove(station) inside _confirmDelete (or the confirmDismiss handler),
return true only if the await succeeds and false on error (showing the existing
error UI), and remove the provider.remove call from onDismissed; apply the same
change to the action-sheet delete path so that the action-sheet handler awaits
provider.remove(...) and only closes/dismisses when the delete completes
successfully.
---
Duplicate comments:
In `@lib/providers/radio_station_provider.dart`:
- Around line 65-68: In remove(RadioStation station) await the async
delete('radio/stations/${station.id}') call and verify success before mutating
local state; if the request fails, surface/log the error and do not call
notifyListeners. When removing from _stations, remove by matching id (e.g. find
and remove where s.id == station.id) instead of _stations.remove(station) to
avoid identity mismatches. Ensure notifyListeners() is only called after
successful deletion and state mutation.
- Line 10: The public getter stations currently returns the backing list
_stations allowing external code (e.g., callers using ..sort(...)) to mutate
provider state without notifyListeners; change the getter implementation in the
provider (the stations getter on the class that holds _stations) to return an
immutable or defensive copy (for example a List.unmodifiable(_stations) or a new
list [..._stations]) so callers cannot modify the internal list directly, and
ensure any sorting/mutations are performed through provider methods that call
notifyListeners() (e.g., expose a sortStations or updateStations method) instead
of modifying the returned list.
In `@lib/ui/screens/radio_stations.dart`:
- Around line 49-50: The current code mutates provider.stations by calling sort
on it during build; instead create a copy and sort that copy (e.g., use toList()
or List.from(provider.stations) into the local variable `stations`) so you don't
modify the provider state; update the `stations` assignment before the sort to
operate on the copied list used in the widget tree.
- Around line 29-38: The _fetchData method currently swallows all exceptions
making failures indistinguishable from an empty list; modify _fetchData to
preserve and surface errors by adding a local state field (e.g., _loadError or
_errorMessage) and set it in the catch block with the caught exception or a
user-friendly message, clear that error on a successful fetch (before or after
calling context.read<RadioStationProvider>().fetchAll()), and leave the finally
block to only reset _loading; also add a retry affordance in the UI that calls
_fetchData so users can retry failed loads. Ensure references to _fetchData and
context.read<RadioStationProvider>().fetchAll() are updated to use the new error
state for rendering an error view instead of the empty-state UI.
- Around line 173-245: The dialog currently pops using the dialog builder
context and then immediately calls showOverlay with that same (now invalid)
context and also closes the form on save failure; change the flow so you call
Navigator.pop(dialogContext) where dialogContext is the builder-provided context
when you need to close the dialog, then after awaiting provider.create(...) and
verifying mounted use the screen's context (this.context) to call showOverlay;
in the catch block do not pop the dialog for the edit dialog path (keep it open
so the user can fix errors), instead show the error overlay using this.context
after a mounted check, and only pop the dialog on successful create/edit
(reference the provider.create call, showOverlay calls, the dialog builder
context variable, Navigator.pop, and mounted).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bc433be-61a5-44bf-8ae8-604d1d9c0207
📒 Files selected for processing (12)
lib/main.dartlib/models/models.dartlib/models/radio_station.dartlib/providers/providers.dartlib/providers/radio_player_provider.dartlib/providers/radio_station_provider.dartlib/ui/screens/library.dartlib/ui/screens/radio_stations.dartlib/ui/screens/screens.dartlib/ui/widgets/mini_player.darttest/models/radio_station_test.darttest/providers/radio_player_provider_test.dart
✅ Files skipped from review due to trivial changes (6)
- lib/models/models.dart
- test/providers/radio_player_provider_test.dart
- lib/ui/screens/screens.dart
- lib/providers/providers.dart
- test/models/radio_station_test.dart
- lib/models/radio_station.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/main.dart
- lib/ui/screens/library.dart
- lib/providers/radio_player_provider.dart
Comment on lines
+12
to
+17
| RadioStationProvider() { | ||
| subscribe(AuthProvider.userLoggedOutStream.listen((_) { | ||
| _stations.clear(); | ||
| notifyListeners(); | ||
| })); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/providers/radio_station_provider.dartRepository: koel/player
Length of output: 2461
🏁 Script executed:
find . -name "stream_subscriber.dart" -o -name "*stream_subscriber*"Repository: koel/player
Length of output: 90
🏁 Script executed:
cat -n lib/mixins/stream_subscriber.dartRepository: koel/player
Length of output: 365
🏁 Script executed:
rg -l "StreamSubscriber" --type dart | head -10Repository: koel/player
Length of output: 454
🏁 Script executed:
for file in lib/providers/playlist_provider.dart lib/providers/podcast_provider.dart lib/providers/recently_played_provider.dart; do
echo "=== $file ===";
grep -A 10 "dispose()" "$file" 2>/dev/null || echo "No dispose() found";
doneRepository: koel/player
Length of output: 252
🏁 Script executed:
rg "void dispose\(\)" lib/providers/ --type dartRepository: koel/player
Length of output: 175
🏁 Script executed:
for file in lib/providers/radio_player_provider.dart lib/providers/download_sync_provider.dart; do
echo "=== $file ===";
head -50 "$file";
doneRepository: koel/player
Length of output: 3105
🏁 Script executed:
for file in lib/providers/radio_player_provider.dart lib/providers/download_sync_provider.dart; do
echo "=== $file ===";
grep -A 10 "void dispose()" "$file";
doneRepository: koel/player
Length of output: 386
🏁 Script executed:
rg "class RadioStationProvider" -A 2 lib/providers/radio_station_provider.dartRepository: koel/player
Length of output: 158
🏁 Script executed:
rg "RadioStationProvider" --type dart -B 2 -A 5 | grep -A 7 "dispose\|RadioStationProvider"Repository: koel/player
Length of output: 3744
🏁 Script executed:
rg "ChangeNotifier" lib/mixins/stream_subscriber.dartRepository: koel/player
Length of output: 37
🏁 Script executed:
rg "class StreamSubscriber" -A 20 lib/mixins/stream_subscriber.dartRepository: koel/player
Length of output: 37
🏁 Script executed:
cat -n lib/mixins/stream_subscriber.dartRepository: koel/player
Length of output: 365
🏁 Script executed:
cd lib && find . -name "*.dart" -exec grep -l "mixin.*ChangeNotifier.*StreamSubscriber\|with ChangeNotifier.*StreamSubscriber\|with StreamSubscriber.*ChangeNotifier" {} \;Repository: koel/player
Length of output: 502
🏁 Script executed:
for file in lib/providers/album_provider.dart lib/providers/favorite_provider.dart lib/providers/interaction_provider.dart; do
echo "=== $file ===";
tail -20 "$file" | head -15;
doneRepository: koel/player
Length of output: 1402
🏁 Script executed:
for file in lib/providers/interaction_provider.dart lib/providers/overview_provider.dart lib/providers/playable_provider.dart; do
echo "=== $file ===";
grep -c "void dispose()" "$file" && echo "Has dispose()" || echo "No dispose()";
doneRepository: koel/player
Length of output: 237
Override dispose() to clean up the logout subscription.
The RadioStationProvider subscribes to AuthProvider.userLoggedOutStream in the constructor but never unsubscribes. When the provider is disposed, this subscription remains active and will trigger notifyListeners() on logout, causing a memory leak and potential errors on a disposed provider.
Suggested fix
RadioStationProvider() {
subscribe(AuthProvider.userLoggedOutStream.listen((_) {
_stations.clear();
notifyListeners();
}));
}
+
+ `@override`
+ void dispose() {
+ unsubscribeAll();
+ super.dispose();
+ }📝 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
| RadioStationProvider() { | |
| subscribe(AuthProvider.userLoggedOutStream.listen((_) { | |
| _stations.clear(); | |
| notifyListeners(); | |
| })); | |
| } | |
| RadioStationProvider() { | |
| subscribe(AuthProvider.userLoggedOutStream.listen((_) { | |
| _stations.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/radio_station_provider.dart` around lines 12 - 17,
RadioStationProvider subscribes to AuthProvider.userLoggedOutStream in the
constructor but never cancels it; add a private StreamSubscription field (e.g.
_logoutSub) to store the result of
subscribe(AuthProvider.userLoggedOutStream.listen(...)) inside the
RadioStationProvider constructor, then override dispose() in
RadioStationProvider to call await or _logoutSub?.cancel() (and set it to null)
before calling super.dispose(), ensuring the subscription is cancelled so
_stations.clear() and notifyListeners() won't run after disposal.
Comment on lines
+13
to
+16
| subscribe(AuthProvider.userLoggedOutStream.listen((_) { | ||
| _stations.clear(); | ||
| notifyListeners(); | ||
| })); |
There was a problem hiding this comment.
Drop late fetchAll() results after logout.
The logout listener clears _stations, but an in-flight fetchAll() still writes its response back afterward. That can repopulate radio stations after logout and leak stale/private data into the next session.
One way to guard late responses
class RadioStationProvider with ChangeNotifier, StreamSubscriber {
var _stations = <RadioStation>[];
+ var _generation = 0;
@@
RadioStationProvider() {
subscribe(AuthProvider.userLoggedOutStream.listen((_) {
+ _generation++;
_stations.clear();
notifyListeners();
}));
}
@@
Future<void> fetchAll() async {
+ final generation = _generation;
final res = await get('radio/stations');
+ if (generation != _generation) return;
+
_stations = (res as List)
.map<RadioStation>((j) => RadioStation.fromJson(j))
.toList();
notifyListeners();
}Also applies to: 19-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/providers/radio_station_provider.dart` around lines 13 - 16, The logout
listener currently clears _stations but doesn't prevent an in-flight fetchAll()
from writing results afterward; add a simple session/request token (e.g. an int
_fetchToken) on the provider, increment it inside the
AuthProvider.userLoggedOutStream.listen handler (where you call
_stations.clear(); notifyListeners();), and in fetchAll() capture the current
token at start and before mutating _stations check that the token still matches
— if it doesn't, drop the response. This keeps the
subscribe(AuthProvider.userLoggedOutStream.listen(...)) logic and
notifyListeners() intact while ensuring late fetchAll() responses are ignored
and cannot repopulate stations after logout.
Comment on lines
+107
to
+113
| confirmDismiss: (_) async { | ||
| return await _confirmDelete( | ||
| context, | ||
| station: station, | ||
| ); | ||
| }, | ||
| onDismissed: (_) => provider.remove(station), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd / && find . -type f -name "radio_stations.dart" 2>/dev/null | head -5Repository: koel/player
Length of output: 109
🏁 Script executed:
wc -l lib/ui/screens/radio_stations.dartRepository: koel/player
Length of output: 93
🏁 Script executed:
sed -n '100,120p' lib/ui/screens/radio_stations.dartRepository: koel/player
Length of output: 1225
🏁 Script executed:
sed -n '275,290p' lib/ui/screens/radio_stations.dartRepository: koel/player
Length of output: 638
🏁 Script executed:
# Find the _confirmDelete method
rg "_confirmDelete" lib/ui/screens/radio_stations.dart -A 10Repository: koel/player
Length of output: 1490
🏁 Script executed:
# Find the provider.remove method definition
fd -t f "*.dart" | xargs rg "provider\.remove|class.*Provider" -A 5 | head -100Repository: koel/player
Length of output: 7923
🏁 Script executed:
# Find the provider file
find lib -type f -name "*provider*" | grep -i radioRepository: koel/player
Length of output: 137
🏁 Script executed:
# Search for RadioStationProvider or similar
rg "class.*RadioStation.*Provider" lib/ -A 5Repository: koel/player
Length of output: 486
🏁 Script executed:
# Search for remove method in providers
rg "Future.*remove\(|void remove\(" lib/ -B 2 -A 8Repository: koel/player
Length of output: 2244
Move the delete operation into confirmDismiss and await it to prevent premature dismissal.
onDismissed is called after the row animation completes, but provider.remove() is async. The row will dismiss before the API call finishes, making errors unrecoverable. Move the delete into confirmDismiss, await it, and return true only on success. Apply the same fix to the action-sheet delete at lines 281-285.
Suggested flow
confirmDismiss: (_) async {
- return await _confirmDelete(
+ final confirmed = await _confirmDelete(
context,
station: station,
);
+
+ if (!confirmed) return false;
+
+ try {
+ await provider.remove(station);
+ return true;
+ } catch (_) {
+ if (!mounted) return false;
+ showOverlay(
+ context,
+ caption: 'Error',
+ icon: CupertinoIcons.exclamationmark_triangle,
+ );
+ return false;
+ }
},
- onDismissed: (_) => provider.remove(station),
@@
- if (confirmed) provider.remove(station);
+ if (!confirmed) return;
+
+ try {
+ await provider.remove(station);
+ } catch (_) {
+ if (!mounted) return;
+ showOverlay(
+ context,
+ caption: 'Error',
+ icon: CupertinoIcons.exclamationmark_triangle,
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/screens/radio_stations.dart` around lines 107 - 113, The dismissal
currently calls provider.remove(...) in onDismissed which lets the row animate
away before the async API call completes; move the deletion into confirmDismiss
by calling and awaiting provider.remove(station) inside _confirmDelete (or the
confirmDismiss handler), return true only if the await succeeds and false on
error (showing the existing error UI), and remove the provider.remove call from
onDismissed; apply the same change to the action-sheet delete path so that the
action-sheet handler awaits provider.remove(...) and only closes/dismisses when
the delete completes successfully.
d99add5 to
197aa85
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
lib/ui/screens/radio_stations.dart (3)
107-113:⚠️ Potential issue | 🟠 MajorWait for deletion to succeed before removing the UI.
lib/providers/radio_station_provider.dart:65-69already makes deletion optimistic, and these two call sites close the row/action sheet immediately on top of that. Wire both paths to an awaitable delete result—move the swipe delete intoconfirmDismiss, await the action-sheet delete, and only dismiss on success so failures can surface.Also applies to: 281-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 107 - 113, The swipe-to-delete currently calls provider.remove in onDismissed immediately; instead move the deletion logic into confirmDismiss by calling and awaiting the provider's delete method (the same awaitable result used by the action-sheet delete flow) from inside _confirmDelete/confirmDismiss, and only return true to allow dismissal when that awaited delete succeeds; also change any action-sheet handlers that call provider.remove to return or expose the same awaitable delete result so failures can be surfaced and the UI only closes on success (update references to _confirmDelete, confirmDismiss, onDismissed, and provider.remove accordingly).
236-245:⚠️ Potential issue | 🟠 MajorUse the screen context for overlays, and keep failed submits open.
After
Navigator.pop(context), the dialog buildercontextis no longer the right context forshowOverlay(). Both failure paths also throw away the user's input; show the error with the screen context after amountedcheck and leave the dialog open so they can retry.Also applies to: 372-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 236 - 245, The dialog's builder context is being used after Navigator.pop and on failures the dialog is closed unnecessarily; update the submit handlers (the block around showOverlay/Navigator.pop and the similar block at 372-376) to capture the surrounding screen context (e.g., declare final screenContext = context before opening the dialog), check mounted before doing UI work, and on success call Navigator.pop(screenContext) then showOverlay(screenContext, caption: 'Station added'); on failure do NOT call Navigator.pop (leave the dialog open), and instead call showOverlay(screenContext, caption: 'Error', icon: CupertinoIcons.exclamationmark_triangle) after the mounted check so the user can retry. Ensure you reference the existing showOverlay and Navigator.pop calls and replace the use of the dialog builder context with the saved screenContext.
33-37:⚠️ Potential issue | 🟠 MajorDon't render load failures as an empty library.
The empty
catchdrops initial-load and refresh errors, so this screen falls through to the"No radio stations"state on failure. Track a load error in state and show an error/retry branch before the empty-state copy.Also applies to: 64-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 33 - 37, The empty catch in the init/load flow swallows fetch errors so the UI shows "No radio stations" instead of an error; update the widget state to track load failures (e.g., add a boolean or error field like _loadError/_loadErrorMessage) and in the catch block set that state via setState (alongside turning off _loading) so the build method can render an error + retry UI; ensure you update both the initial fetch call (where context.read<RadioStationProvider>().fetchAll() is awaited) and the refresh path to set/clear the error state, and use mounted checks before calling setState.
🧹 Nitpick comments (2)
lib/ui/widgets/mini_player.dart (2)
175-183: Consider adding accessibility labels to the play/pause button.The
IconButtonlacks atooltipor wrappingSemanticswidget, which may impact screen reader accessibility. Consider adding a tooltip that reflects the current state.♿ Suggested accessibility improvement
IconButton( + tooltip: radioPlayer.playing ? 'Pause' : 'Play', onPressed: radioPlayer.togglePlayPause, icon: Icon( radioPlayer.playing ? CupertinoIcons.pause_fill : CupertinoIcons.play_fill, size: 24, ), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/mini_player.dart` around lines 175 - 183, Add accessible labeling to the play/pause IconButton by providing a tooltip or wrapping it in a Semantics widget that reflects the current state; update the IconButton (the widget using radioPlayer.togglePlayPause and radioPlayer.playing) to supply a tooltip string like "Play" or "Pause" based on radioPlayer.playing or wrap the button in Semantics(label: ...) with the same dynamic text so screen readers announce the current action.
195-203: Optional: SimplifyisLoadingassignment.The
late finalpattern with conditional assignment can be simplified to a single expression.✨ More concise alternative
- late final bool isLoading; - - if ((state.processingState == AudioProcessingState.buffering || - state.processingState == AudioProcessingState.loading) && - state.playing) { - isLoading = true; - } else { - isLoading = false; - } + final isLoading = state.playing && + (state.processingState == AudioProcessingState.buffering || + state.processingState == AudioProcessingState.loading);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/mini_player.dart` around lines 195 - 203, The isLoading variable is declared as late final and set via an if/else; simplify by replacing that pattern with a single boolean expression (e.g., assign isLoading = (state.processingState == AudioProcessingState.buffering || state.processingState == AudioProcessingState.loading) && state.playing) to make the intent clearer and remove the late modifier; update the declaration of isLoading accordingly where it appears in the widget.
🤖 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/radio_stations.dart`:
- Around line 107-113: The swipe-to-delete currently calls provider.remove in
onDismissed immediately; instead move the deletion logic into confirmDismiss by
calling and awaiting the provider's delete method (the same awaitable result
used by the action-sheet delete flow) from inside _confirmDelete/confirmDismiss,
and only return true to allow dismissal when that awaited delete succeeds; also
change any action-sheet handlers that call provider.remove to return or expose
the same awaitable delete result so failures can be surfaced and the UI only
closes on success (update references to _confirmDelete, confirmDismiss,
onDismissed, and provider.remove accordingly).
- Around line 236-245: The dialog's builder context is being used after
Navigator.pop and on failures the dialog is closed unnecessarily; update the
submit handlers (the block around showOverlay/Navigator.pop and the similar
block at 372-376) to capture the surrounding screen context (e.g., declare final
screenContext = context before opening the dialog), check mounted before doing
UI work, and on success call Navigator.pop(screenContext) then
showOverlay(screenContext, caption: 'Station added'); on failure do NOT call
Navigator.pop (leave the dialog open), and instead call
showOverlay(screenContext, caption: 'Error', icon:
CupertinoIcons.exclamationmark_triangle) after the mounted check so the user can
retry. Ensure you reference the existing showOverlay and Navigator.pop calls and
replace the use of the dialog builder context with the saved screenContext.
- Around line 33-37: The empty catch in the init/load flow swallows fetch errors
so the UI shows "No radio stations" instead of an error; update the widget state
to track load failures (e.g., add a boolean or error field like
_loadError/_loadErrorMessage) and in the catch block set that state via setState
(alongside turning off _loading) so the build method can render an error + retry
UI; ensure you update both the initial fetch call (where
context.read<RadioStationProvider>().fetchAll() is awaited) and the refresh path
to set/clear the error state, and use mounted checks before calling setState.
---
Nitpick comments:
In `@lib/ui/widgets/mini_player.dart`:
- Around line 175-183: Add accessible labeling to the play/pause IconButton by
providing a tooltip or wrapping it in a Semantics widget that reflects the
current state; update the IconButton (the widget using
radioPlayer.togglePlayPause and radioPlayer.playing) to supply a tooltip string
like "Play" or "Pause" based on radioPlayer.playing or wrap the button in
Semantics(label: ...) with the same dynamic text so screen readers announce the
current action.
- Around line 195-203: The isLoading variable is declared as late final and set
via an if/else; simplify by replacing that pattern with a single boolean
expression (e.g., assign isLoading = (state.processingState ==
AudioProcessingState.buffering || state.processingState ==
AudioProcessingState.loading) && state.playing) to make the intent clearer and
remove the late modifier; update the declaration of isLoading accordingly where
it appears in the widget.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b4b6ec6-d05c-429b-8cfd-d3e0b80b4762
📒 Files selected for processing (12)
lib/main.dartlib/models/models.dartlib/models/radio_station.dartlib/providers/providers.dartlib/providers/radio_player_provider.dartlib/providers/radio_station_provider.dartlib/ui/screens/library.dartlib/ui/screens/radio_stations.dartlib/ui/screens/screens.dartlib/ui/widgets/mini_player.darttest/models/radio_station_test.darttest/providers/radio_player_provider_test.dart
✅ Files skipped from review due to trivial changes (6)
- test/providers/radio_player_provider_test.dart
- lib/providers/providers.dart
- lib/models/models.dart
- lib/ui/screens/screens.dart
- test/models/radio_station_test.dart
- lib/models/radio_station.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/main.dart
- lib/ui/screens/library.dart
- lib/providers/radio_player_provider.dart
- lib/providers/radio_station_provider.dart
phanan
force-pushed
the
feat/radio-stations
branch
3 times, most recently
from
e90196e to
8f63d04
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (10)
lib/providers/radio_station_provider.dart (4)
19-25:⚠️ Potential issue | 🟠 MajorGuard against late responses after logout.
An in-flight
fetchAll()can repopulate_stationsafter logout clears them, potentially leaking stale data into the next session.Proposed fix with generation tracking
class RadioStationProvider with ChangeNotifier, StreamSubscriber { var _stations = <RadioStation>[]; + var _generation = 0; List<RadioStation> get stations => _stations; RadioStationProvider() { subscribe(AuthProvider.userLoggedOutStream.listen((_) { + _generation++; _stations.clear(); notifyListeners(); })); } Future<void> fetchAll() async { + final generation = _generation; final res = await get('radio/stations'); + if (generation != _generation) return; + _stations = (res as List) .map<RadioStation>((j) => RadioStation.fromJson(j)) .toList(); notifyListeners(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/radio_station_provider.dart` around lines 19 - 25, fetchAll can repopulate _stations after logout; fix by adding a generation/token check: introduce a private counter (e.g., _fetchGeneration) on the provider, capture its value at the start of fetchAll (localGen), and before assigning to _stations and calling notifyListeners verify the current _fetchGeneration equals localGen; when you clear or logout (the method that clears _stations) increment _fetchGeneration so any in-flight fetches are ignored. Apply changes around the fetchAll, _stations assignment and the logout/clear method to ensure stale responses are dropped.
12-17:⚠️ Potential issue | 🟠 MajorOverride
dispose()to clean up the logout subscription.The provider subscribes to
AuthProvider.userLoggedOutStreambut never unsubscribes. When disposed, this subscription remains active and can triggernotifyListeners()on a disposed provider.Proposed fix
RadioStationProvider() { subscribe(AuthProvider.userLoggedOutStream.listen((_) { _stations.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/radio_station_provider.dart` around lines 12 - 17, RadioStationProvider currently subscribes to AuthProvider.userLoggedOutStream in its constructor via subscribe(...) but never cancels that subscription on teardown; add an override of dispose() (or close()/detach if using a base lifecycle) that cancels/unsubscribes the stream subscription created in RadioStationProvider (store the returned StreamSubscription from subscribe/AuthProvider.userLoggedOutStream.listen), then call super.dispose() after cancelling, ensuring any call to _stations.clear() and notifyListeners() will not run on a disposed provider.
10-10:⚠️ Potential issue | 🟠 MajorDo not expose mutable provider state directly.
The getter returns the backing list directly, allowing callers to mutate
_stationswithout triggeringnotifyListeners().Proposed fix
+import 'dart:collection'; @@ - List<RadioStation> get stations => _stations; + UnmodifiableListView<RadioStation> get stations => + UnmodifiableListView(_stations);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/radio_station_provider.dart` at line 10, The public getter stations returns the internal mutable list _stations allowing external mutation; change the getter to return an immutable view/copy (e.g., List.unmodifiable(_stations) or a defensive copy) so callers cannot modify the backing list directly, and keep all mutations confined to the provider methods that call notifyListeners(); update the stations getter (and any other public accessor exposing _stations) to return a non-mutable list and ensure only provider methods mutate _stations.
70-74:⚠️ Potential issue | 🔴 CriticalAwait delete before mutating local state.
The server delete is fire-and-forget. If it fails, local state diverges from server state.
Proposed fix
Future<void> remove(RadioStation station) async { - delete('radio/stations/${station.id}'); - _stations.remove(station); + await delete('radio/stations/${station.id}'); + _stations.removeWhere((s) => s.id == station.id); notifyListeners(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/radio_station_provider.dart` around lines 70 - 74, In remove(RadioStation station) await the server delete Future returned by delete('radio/stations/${station.id}') and only mutate local state after it completes successfully; wrap the await in a try/catch so on success you call _stations.remove(station) and notifyListeners(), and on failure log or rethrow the error to avoid diverging local state from the server.lib/ui/screens/radio_stations.dart (6)
239-258:⚠️ Potential issue | 🟠 MajorUse screen context for feedback after dialog close.
After
Navigator.pop(context), the dialog's builder context is invalid for overlay operations. Use the screen's context with a mounted check. Additionally, consider keeping the dialog open on failure so the user can retry.Proposed fix
- builder: (context) => CupertinoAlertDialog( + builder: (dialogContext) => CupertinoAlertDialog( @@ - Navigator.pop(context); - showOverlay(context, caption: 'Station added'); + Navigator.pop(dialogContext); + if (!mounted) return; + showOverlay(this.context, caption: 'Station added'); } catch (e) { - Navigator.pop(context); var message = 'Something went wrong.'; if (e is HttpResponseException) { try { final body = jsonDecode(e.response.body); if (body['message'] != null) { message = body['message']; } } catch (_) {} } + if (!mounted) return; showOverlay( - context, + this.context, caption: 'Error', message: message, icon: CupertinoIcons.exclamationmark_triangle, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 239 - 258, The overlay is being shown using the dialog's builder context after calling Navigator.pop(context), which can be invalid; instead, only call Navigator.pop(context) on success (move Navigator.pop(context) into the try-success branch) and use the screen's valid context (check mounted before showing overlays) when calling showOverlay; keep the dialog open on failure so the user can retry, and retain the existing HttpResponseException handling/JSON parsing to compute the message but call showOverlay with the screen context (and only if mounted).
52-53:⚠️ Potential issue | 🟠 MajorIn-place sort mutates provider's backing list on every build.
The cascade
provider.stations..sort(...)modifies the provider's internal list. Create a sorted copy instead.Proposed fix
- final stations = provider.stations - ..sort((a, b) => a.name.compareTo(b.name)); + final stations = [...provider.stations] + ..sort((a, b) => a.name.compareTo(b.name));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 52 - 53, The code mutates the provider's backing list by using the cascade sort on provider.stations; instead, create a copy and sort that copy (e.g., use provider.stations.toList() or List.from(provider.stations) and then sort) so replace the current `final stations = provider.stations..sort((a, b) => a.name.compareTo(b.name));` with a non-mutating version that sorts the copied list, leaving provider.stations unchanged.
110-116:⚠️ Potential issue | 🟠 MajorMove delete operation into
confirmDismissto prevent premature dismissal.
onDismissedfires after the row animation, butprovider.remove()is async. If the API fails, the row is already gone and the error is unrecoverable. Move the delete intoconfirmDismiss, await it, and returntrueonly on success.Proposed fix
confirmDismiss: (_) async { - return await _confirmDelete( + final confirmed = await _confirmDelete( context, station: station, ); + if (!confirmed) return false; + + try { + await provider.remove(station); + return true; + } catch (_) { + if (!mounted) return false; + showOverlay( + context, + caption: 'Error', + icon: CupertinoIcons.exclamationmark_triangle, + ); + return false; + } }, - onDismissed: (_) => provider.remove(station), + onDismissed: (_) {},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 110 - 116, The current flow calls provider.remove in onDismissed which runs after the UI row animates away; instead move the async delete into confirmDismiss so the Dismissible only dismisses on success: inside confirmDismiss (where _confirmDelete is invoked) perform the provider.remove(station) call, await it, handle errors (e.g., show an error/snackbar) and return true only if removal succeeded; remove or clear the onDismissed callback to avoid duplicating deletion and ensure the UI only dismisses when provider.remove completes successfully.
385-389:⚠️ Potential issue | 🟠 MajorKeep edit dialog open on failure and show error feedback.
On failure, the dialog closes silently without feedback, discarding the user's edits and leaving no retry option. Use the screen context and keep the dialog open on error.
Proposed fix
- builder: (context) => CupertinoAlertDialog( + builder: (dialogContext) => CupertinoAlertDialog( @@ - Navigator.pop(context); - showOverlay(context, caption: 'Station updated'); + Navigator.pop(dialogContext); + if (!mounted) return; + showOverlay(this.context, caption: 'Station updated'); } catch (_) { - Navigator.pop(context); + if (!mounted) return; + showOverlay( + this.context, + caption: 'Error', + icon: CupertinoIcons.exclamationmark_triangle, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 385 - 389, The catch block currently swallows errors and closes the edit dialog via Navigator.pop(context); instead capture the error (change catch (_) to catch (e)) and remove the Navigator.pop(context) call so the dialog stays open; then surface feedback using showOverlay (or your existing screen-level feedback helper) with a descriptive message like "Failed to update station: <error>" using the screen/screen-level context (not the dialog context) so the user sees the failure and can retry; keep the successful path calling showOverlay(context, caption: 'Station updated') unchanged.
32-42:⚠️ Potential issue | 🟠 MajorSwallowing fetch failures hides errors as empty state.
The catch block is empty, so failed fetches silently fall through to display "No radio stations" even when the user has stations but the API failed.
Proposed fix: track error state
class _RadioStationsScreenState extends State<RadioStationsScreen> { var _loading = false; + String? _error; @@ Future<void> _fetchData() async { if (_loading) return; - setState(() => _loading = true); + setState(() { + _loading = true; + _error = null; + }); try { await context.read<RadioStationProvider>().fetchAll(); - } catch (_) { + } catch (e) { + if (mounted) setState(() => _error = 'Could not load stations.'); } finally { if (mounted) setState(() => _loading = false); } }Then render the error state in the UI when
_error != null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 32 - 42, The fetch method _fetchData currently swallows exceptions causing silent empty UI; add an instance field (e.g., _error) to track fetch errors, clear _error before calling context.read<RadioStationProvider>().fetchAll(), catch the exception in the catch block and set _error to the caught error or a user-friendly message inside setState, and ensure finally still clears _loading only after mounted; then update the widget's build logic to render an error state when _error != null instead of showing the "No radio stations" empty state.
294-299:⚠️ Potential issue | 🟠 MajorAwait delete and handle errors in action sheet.
The delete is fire-and-forget with no error handling. Apply the same fix as the swipe-to-delete flow.
Proposed fix
onPressed: () async { Navigator.pop(sheetContext); final confirmed = await _confirmDelete(context, station: station); - if (confirmed) provider.remove(station); + if (!confirmed) return; + + try { + await provider.remove(station); + } catch (_) { + if (!mounted) return; + showOverlay( + context, + caption: 'Error', + icon: CupertinoIcons.exclamationmark_triangle, + ); + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 294 - 299, The action-sheet delete currently fires provider.remove(station) without awaiting or handling errors; modify the onPressed handler (the closure using sheetContext and calling _confirmDelete(context, station)) to await the remove call and wrap it in try/catch: after confirming, call await provider.remove(station) inside a try block and catch exceptions to show an error to the user (e.g., via ScaffoldMessenger.of(context).showSnackBar) and/or log the error; ensure control flow/popping uses sheetContext consistently (pop only after successful delete or still pop but surface delete errors to the user).
🤖 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/providers/radio_station_provider.dart`:
- Around line 19-25: fetchAll can repopulate _stations after logout; fix by
adding a generation/token check: introduce a private counter (e.g.,
_fetchGeneration) on the provider, capture its value at the start of fetchAll
(localGen), and before assigning to _stations and calling notifyListeners verify
the current _fetchGeneration equals localGen; when you clear or logout (the
method that clears _stations) increment _fetchGeneration so any in-flight
fetches are ignored. Apply changes around the fetchAll, _stations assignment and
the logout/clear method to ensure stale responses are dropped.
- Around line 12-17: RadioStationProvider currently subscribes to
AuthProvider.userLoggedOutStream in its constructor via subscribe(...) but never
cancels that subscription on teardown; add an override of dispose() (or
close()/detach if using a base lifecycle) that cancels/unsubscribes the stream
subscription created in RadioStationProvider (store the returned
StreamSubscription from subscribe/AuthProvider.userLoggedOutStream.listen), then
call super.dispose() after cancelling, ensuring any call to _stations.clear()
and notifyListeners() will not run on a disposed provider.
- Line 10: The public getter stations returns the internal mutable list
_stations allowing external mutation; change the getter to return an immutable
view/copy (e.g., List.unmodifiable(_stations) or a defensive copy) so callers
cannot modify the backing list directly, and keep all mutations confined to the
provider methods that call notifyListeners(); update the stations getter (and
any other public accessor exposing _stations) to return a non-mutable list and
ensure only provider methods mutate _stations.
- Around line 70-74: In remove(RadioStation station) await the server delete
Future returned by delete('radio/stations/${station.id}') and only mutate local
state after it completes successfully; wrap the await in a try/catch so on
success you call _stations.remove(station) and notifyListeners(), and on failure
log or rethrow the error to avoid diverging local state from the server.
In `@lib/ui/screens/radio_stations.dart`:
- Around line 239-258: The overlay is being shown using the dialog's builder
context after calling Navigator.pop(context), which can be invalid; instead,
only call Navigator.pop(context) on success (move Navigator.pop(context) into
the try-success branch) and use the screen's valid context (check mounted before
showing overlays) when calling showOverlay; keep the dialog open on failure so
the user can retry, and retain the existing HttpResponseException handling/JSON
parsing to compute the message but call showOverlay with the screen context (and
only if mounted).
- Around line 52-53: The code mutates the provider's backing list by using the
cascade sort on provider.stations; instead, create a copy and sort that copy
(e.g., use provider.stations.toList() or List.from(provider.stations) and then
sort) so replace the current `final stations = provider.stations..sort((a, b) =>
a.name.compareTo(b.name));` with a non-mutating version that sorts the copied
list, leaving provider.stations unchanged.
- Around line 110-116: The current flow calls provider.remove in onDismissed
which runs after the UI row animates away; instead move the async delete into
confirmDismiss so the Dismissible only dismisses on success: inside
confirmDismiss (where _confirmDelete is invoked) perform the
provider.remove(station) call, await it, handle errors (e.g., show an
error/snackbar) and return true only if removal succeeded; remove or clear the
onDismissed callback to avoid duplicating deletion and ensure the UI only
dismisses when provider.remove completes successfully.
- Around line 385-389: The catch block currently swallows errors and closes the
edit dialog via Navigator.pop(context); instead capture the error (change catch
(_) to catch (e)) and remove the Navigator.pop(context) call so the dialog stays
open; then surface feedback using showOverlay (or your existing screen-level
feedback helper) with a descriptive message like "Failed to update station:
<error>" using the screen/screen-level context (not the dialog context) so the
user sees the failure and can retry; keep the successful path calling
showOverlay(context, caption: 'Station updated') unchanged.
- Around line 32-42: The fetch method _fetchData currently swallows exceptions
causing silent empty UI; add an instance field (e.g., _error) to track fetch
errors, clear _error before calling
context.read<RadioStationProvider>().fetchAll(), catch the exception in the
catch block and set _error to the caught error or a user-friendly message inside
setState, and ensure finally still clears _loading only after mounted; then
update the widget's build logic to render an error state when _error != null
instead of showing the "No radio stations" empty state.
- Around line 294-299: The action-sheet delete currently fires
provider.remove(station) without awaiting or handling errors; modify the
onPressed handler (the closure using sheetContext and calling
_confirmDelete(context, station)) to await the remove call and wrap it in
try/catch: after confirming, call await provider.remove(station) inside a try
block and catch exceptions to show an error to the user (e.g., via
ScaffoldMessenger.of(context).showSnackBar) and/or log the error; ensure control
flow/popping uses sheetContext consistently (pop only after successful delete or
still pop but surface delete errors to the user).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f3ac036-5d9f-440c-95d8-ee5153303918
📒 Files selected for processing (12)
lib/main.dartlib/models/models.dartlib/models/radio_station.dartlib/providers/providers.dartlib/providers/radio_player_provider.dartlib/providers/radio_station_provider.dartlib/ui/screens/library.dartlib/ui/screens/radio_stations.dartlib/ui/screens/screens.dartlib/ui/widgets/mini_player.darttest/models/radio_station_test.darttest/providers/radio_player_provider_test.dart
✅ Files skipped from review due to trivial changes (5)
- lib/ui/screens/screens.dart
- lib/providers/providers.dart
- test/providers/radio_player_provider_test.dart
- lib/models/radio_station.dart
- test/models/radio_station_test.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/models/models.dart
- lib/ui/screens/library.dart
- lib/main.dart
- lib/providers/radio_player_provider.dart
- RadioStation model with fromJson, fake factory - RadioStationProvider with CRUD operations (fetch, create, update, delete) and now-playing metadata endpoint - RadioStationsScreen with list, add/edit/delete dialogs, long-press actions - Play radio by streaming through the proxy endpoint with audio token - Station rows show logo (circular), name, description, antenna icon - Swipe-to-delete with confirmation dialog - Empty state with icon and message - Added to Library screen navigation - Registered provider in main.dart
phanan
force-pushed
the
feat/radio-stations
branch
from
8f63d04 to
7e27df7
Compare
This was referenced Mar 28, 2026
This was referenced Apr 7, 2026
This was referenced May 1, 2026
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
id,name,url,logo,description,isPublicfieldsfetchAll,create,update,remove,getNowPlaying/radio/stream/{id}?t=audioToken)RadioStationProviderinmain.dartTest plan
Summary by CodeRabbit
New Features
Tests