Redesign Play/Shuffle/Search controls for song lists#161
Merged
Conversation
Replace the old icon-based play/shuffle/search controls with: - Two side-by-side rounded Play and Shuffle buttons (Apple Music style) - A search field that slides in when scrolling down and hides when scrolling back up, using AnimatedCrossFade for smooth transitions The scroll-aware search behavior is driven by the scroll controller passed from each screen.
|
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:
📝 WalkthroughWalkthroughWires persistent ScrollController instances into multiple screens and forwards them to PlayableListHeader; refactors PlayableListHeader to accept an optional scrollController, remove search expand/collapse callbacks and icon params, and simplify search UI/state and controller lifecycle. Changes
Sequence DiagramsequenceDiagram
participant User
participant Screen as "Screen\n(CustomScrollView)"
participant Controller as "ScrollController"
participant Header as "PlayableListHeader"
participant Search as "Search Field"
User->>Screen: scroll / interact
Screen->>Controller: update position
Controller->>Header: shared scroll updates
User->>Header: open search / tap buttons
Header->>Search: show / hide / clear search field
Search->>User: input / cancel
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Make Play/Shuffle buttons fully rounded (pill shape) with borderRadius 100. Attach _scrollController to CustomScrollView in 4 screens where it was missing, so the scroll-aware search field works.
Show search when scrolling up (negative delta), hide when scrolling down (positive delta), matching Apple Music behavior where the search field is above the list content.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ui/screens/songs.dart (1)
149-160:⚠️ Potential issue | 🟠 MajorSearch-mode state is no longer updated, which can break search requests.
After this refactor,
_inSearchModeis never toggled by header events, but request/pagination logic still depends on it (e.g.,makeRequest()early return whenpage == null). This can cause search to stop working in common flows.💡 Proposed fix
@@ SliverToBoxAdapter( child: SongListHeader( sortField: _paginationConfig.sortField, sortOrder: _paginationConfig.sortOrder, scrollController: _scrollController, - onSearchExpanded: () => - setState(() => _inSearchMode = true), - onSearchCollapsed: () => setState( - () => _inSearchMode = false, - ), onSearchQueryChanged: (query) { - setState(() => _searchQuery = query); + setState(() { + _searchQuery = query; + _inSearchMode = query.trim().isNotEmpty; + }); makeRequest(); }, ), ), @@ class SongListHeader extends StatefulWidget { final String sortField; final SortOrder sortOrder; final ScrollController? scrollController; final Function(String) onSearchQueryChanged; - final Function() onSearchExpanded; - final Function() onSearchCollapsed; @@ const SongListHeader({ @@ required this.sortOrder, this.scrollController, required this.onSearchQueryChanged, - required this.onSearchExpanded, - required this.onSearchCollapsed, }) : super(key: key);Also applies to: 196-212, 249-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/songs.dart` around lines 149 - 160, The header callbacks no longer toggle the _inSearchMode state, breaking search flows; update the SongListHeader event handlers (onSearchExpanded and onSearchCollapsed) to call setState to set _inSearchMode = true/false respectively and ensure onSearchQueryChanged still updates _searchQuery and calls makeRequest(); verify any other header-related handlers (the occurrences around lines with SongListHeader and similar callbacks) also update _inSearchMode so pagination logic in makeRequest() sees the correct search mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/screens/album_details.dart`:
- Line 23: The StatefulWidget in album_details.dart creates a ScrollController
named _scrollController and uses it in the CustomScrollView but never releases
it; add an override of dispose() in the State class that calls
_scrollController.dispose() and then super.dispose() to properly clean up the
controller (ensure the dispose method is added to the same State that declares
_scrollController and tied to the CustomScrollView).
In `@lib/ui/screens/downloaded.dart`:
- Line 24: Add a dispose() override in the StatefulWidget's State that owns the
_scrollController (the same State used for the CustomScrollView and
PlayableListHeader) and call _scrollController.dispose() there, then call
super.dispose(); this ensures the ScrollController is released when the
screen/state is destroyed.
In `@lib/ui/screens/podcast_details.dart`:
- Line 24: The state owns a ScrollController (_scrollController) but doesn't
dispose it; add an override dispose() method in the State class that calls
_scrollController.dispose() and then super.dispose() to release resources when
the widget is destroyed (place the method inside the same State that declares
_scrollController).
In `@lib/ui/screens/recently_played.dart`:
- Line 21: The _scrollController is created on the State but never disposed; add
an override of dispose() in the State subclass that declares _scrollController
(the same class where you defined final _scrollController = ScrollController())
and call _scrollController.dispose() before calling super.dispose(); this will
properly clean up the controller and prevent leaks.
In `@lib/ui/widgets/playable_list_header.dart`:
- Around line 54-55: The listener on widget.scrollController calling _onScroll
treats programmatic jumps (e.g. AlphabetScrollbar.jumpTo()) as user scrolls;
change to only react to real user-initiated scrolling by routing scroll handling
through a NotificationListener<UserScrollNotification> (or check for
UserScrollNotification in your scroll notifications) and call _onScroll only
when the notification indicates user scrolling (not programmatic). Update the
widget to stop relying solely on widget.scrollController?.addListener(_onScroll)
and instead handle scroll events in the build via
NotificationListener<UserScrollNotification> (or set/clear a flag around
jumpTo() in AlphabetScrollbar) so _onScroll ignores programmatic jumpTo()
movements.
---
Outside diff comments:
In `@lib/ui/screens/songs.dart`:
- Around line 149-160: The header callbacks no longer toggle the _inSearchMode
state, breaking search flows; update the SongListHeader event handlers
(onSearchExpanded and onSearchCollapsed) to call setState to set _inSearchMode =
true/false respectively and ensure onSearchQueryChanged still updates
_searchQuery and calls makeRequest(); verify any other header-related handlers
(the occurrences around lines with SongListHeader and similar callbacks) also
update _inSearchMode so pagination logic in makeRequest() sees the correct
search mode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 705767f7-25dc-44ad-8de8-350fb7e7251e
📒 Files selected for processing (10)
lib/ui/screens/album_details.dartlib/ui/screens/artist_details.dartlib/ui/screens/downloaded.dartlib/ui/screens/favorites.dartlib/ui/screens/genre_details.dartlib/ui/screens/playlist_details.dartlib/ui/screens/podcast_details.dartlib/ui/screens/recently_played.dartlib/ui/screens/songs.dartlib/ui/widgets/playable_list_header.dart
…release Accumulate scroll delta in each direction and only toggle the search field after crossing a threshold (30px to show, 50px to hide). This prevents the small bounce on finger release from immediately hiding the search.
Add a magnifier icon button to the left of Play/Shuffle. Tapping it cross-fades the buttons into a search field + Cancel button. Cancel clears the search and restores the buttons. Removes all scroll controller logic.
…witcher Both the buttons row and search row now share the same fixed 48px height inside an AnimatedSwitcher, eliminating the height interpolation that caused the layout jump.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/widgets/playable_list_header.dart`:
- Around line 23-31: PlayableListHeader currently accepts a scrollController but
never uses it, so the _searching visibility only toggles via button taps; wire
the provided scrollController into the widget lifecycle: in the State for
PlayableListHeader attach a listener to widget.scrollController (or to an
internal controller if null) in initState, reattach in didUpdateWidget when the
controller instance changes, and remove the listener in dispose; in the listener
compute scroll direction/position and call setState to set _searching true when
scrolling down past a threshold and false when scrolling up (and when the search
query is empty), ensuring you guard with mounted and only attach a single
listener to avoid leaks.
- Around line 24-35: The PlayableListHeader currently updates local search state
only, breaking parent screens that rely on expand/collapse events; add two
optional callbacks (e.g., Function()? onSearchOpened and Function()?
onSearchClosed) to the PlayableListHeader constructor and class fields, and
invoke onSearchOpened() inside _openSearch() and onSearchClosed() inside
_closeSearch() (with null checks) so parent screens like Songs can receive the
expand/collapse signals and toggle their _inSearchMode as before; update any
header instantiations to pass handlers where needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de6220f1-8e4b-4767-bccc-96a30ef9187c
📒 Files selected for processing (1)
lib/ui/widgets/playable_list_header.dart
The search magnifier button smoothly expands to the right into a full-width pill containing the text field and an X close button. Play/Shuffle buttons fade out and shrink as the search pill grows. Collapsing reverses the animation.
…ffle Pick the background image based on day-of-month modulo candidate count so the same list always returns the same image within a day. Prevents the cover from changing on every rebuild caused by search queries. Also update button label colors to Colors.white70.
The paginated provider grows the list over time, changing the modulo result. Cache the background widget so it's only computed once.
phanan
changed the title
phanan
changed the title
Add onSearchOpened/onSearchClosed callbacks to PlayableListHeader so the Songs screen can toggle _inSearchMode to prevent pagination during search. Also dispose _scrollController in album details.
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