Highlight currently playing radio station#149
Merged
Conversation
📝 WalkthroughWalkthroughThe radio stations screen's row widget now subscribes to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/ui/screens/radio_stations_test.dart (2)
88-95: Test covers an important edge case but assertion is incomplete.This test verifies the scenario where a different station is actively playing (
playing = truebutisPlaying = false). The assertion only checksisPlaying, but doesn't verify thatloading || playingis intentionally ignored whenisPlayingis false.Consider adding a clarifying comment or assertion:
// Animation condition is true, but overlay is not shown because station is not active expect(loading || playing, isTrue); // Would show animation IF this were the active station expect(isPlaying, isFalse); // But it's not, so no overlay at all🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/screens/radio_stations_test.dart` around lines 88 - 95, The test "no overlay when station is not active" currently only asserts isPlaying is false; update the test to also assert that the animation condition is true by adding an expectation for (loading || playing) to be true (and include a short comment noting "Animation condition is true, but overlay is not shown because station is not active") so the test verifies that loading || playing would trigger an animation for an active station but that no overlay appears when isPlaying is false; refer to the test name and the variables isPlaying, loading, and playing when making the changes.
4-96: These tests verify boolean/string semantics, not widget behavior.The current tests only validate that
==works on String IDs and||works on booleans—essentially testing Dart language primitives. They don't actually test the_RadioStationRowwidget implementation.Consider adding widget tests that:
- Pump
_RadioStationRowwith a mockedRadioPlayerProvider- Verify the overlay
Containerappears when the station is active- Verify the animation
Image.assetappears whenplaying || loading- Verify
AppColors.highlightis applied to title/icon when active- Verify state changes trigger rebuilds correctly
Example widget test structure
import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:mockito/mockito.dart'; import 'package:provider/provider.dart'; // ... other imports class MockRadioPlayerProvider extends Mock implements RadioPlayerProvider {} void main() { group('_RadioStationRow widget', () { testWidgets('shows overlay and animation when station is playing', (tester) async { final mockProvider = MockRadioPlayerProvider(); final station = RadioStation.fake(id: 'station-1'); when(mockProvider.currentStation).thenReturn(station); when(mockProvider.playing).thenReturn(true); when(mockProvider.loading).thenReturn(false); await tester.pumpWidget( MaterialApp( home: ChangeNotifierProvider<RadioPlayerProvider>.value( value: mockProvider, child: Scaffold( body: _RadioStationRow(station: station, onTap: () {}), ), ), ), ); // Verify overlay container exists expect(find.byType(Container), findsWidgets); // Verify animation asset is shown expect(find.byType(Image), findsOneWidget); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/screens/radio_stations_test.dart` around lines 4 - 96, Replace the primitive boolean/string assertions with real widget tests: pump the _RadioStationRow widget inside a MaterialApp and a ChangeNotifierProvider<RadioPlayerProvider> backed by a mocked provider (mock methods: currentStation, playing, loading), then assert the overlay Container is present when currentStation matches the row's RadioStation, assert the animation Image.asset is present when (playing || loading) is true, assert title/icon Text/Icon widgets use AppColors.highlight when active, and verify changing the mocked provider state and calling tester.pump()/pumpAndSettle() triggers the row to rebuild and update visibility/styles accordingly.lib/ui/screens/radio_stations.dart (1)
424-426: Consider renamingisPlayingtoisActivefor clarity.The variable
isPlayingis misleading—it represents whether this station is the currently selected station, not whether audio is actively playing. The actual playback state is accessed viaradioPlayer.playing. This naming confusion could lead to maintenance issues.Suggested rename
return Consumer<RadioPlayerProvider>( builder: (context, radioPlayer, _) { - final isPlaying = radioPlayer.currentStation?.id == station.id; + final isActive = radioPlayer.currentStation?.id == station.id;Then update all references to
isPlaying→isActivewithin this builder (lines 452, 458, 474, 489).🤖 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 424 - 426, Rename the local boolean variable isPlaying to isActive in the Consumer<RadioPlayerProvider> builder where it's set as radioPlayer.currentStation?.id == station.id, and update all uses of isPlaying within that builder to isActive (while keeping the actual playback flag radioPlayer.playing untouched) so the name reflects "currently selected station" rather than playback state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/ui/screens/radio_stations.dart`:
- Around line 424-426: Rename the local boolean variable isPlaying to isActive
in the Consumer<RadioPlayerProvider> builder where it's set as
radioPlayer.currentStation?.id == station.id, and update all uses of isPlaying
within that builder to isActive (while keeping the actual playback flag
radioPlayer.playing untouched) so the name reflects "currently selected station"
rather than playback state.
In `@test/ui/screens/radio_stations_test.dart`:
- Around line 88-95: The test "no overlay when station is not active" currently
only asserts isPlaying is false; update the test to also assert that the
animation condition is true by adding an expectation for (loading || playing) to
be true (and include a short comment noting "Animation condition is true, but
overlay is not shown because station is not active") so the test verifies that
loading || playing would trigger an animation for an active station but that no
overlay appears when isPlaying is false; refer to the test name and the
variables isPlaying, loading, and playing when making the changes.
- Around line 4-96: Replace the primitive boolean/string assertions with real
widget tests: pump the _RadioStationRow widget inside a MaterialApp and a
ChangeNotifierProvider<RadioPlayerProvider> backed by a mocked provider (mock
methods: currentStation, playing, loading), then assert the overlay Container is
present when currentStation matches the row's RadioStation, assert the animation
Image.asset is present when (playing || loading) is true, assert title/icon
Text/Icon widgets use AppColors.highlight when active, and verify changing the
mocked provider state and calling tester.pump()/pumpAndSettle() triggers the row
to rebuild and update visibility/styles accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eeb23f2b-eac1-4678-9a0f-13356abfe7bf
📒 Files selected for processing (2)
lib/ui/screens/radio_stations.darttest/ui/screens/radio_stations_test.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
Test plan
flutter test test/ui/screens/radio_stations_test.dartSummary by CodeRabbit
Release Notes
New Features
Tests