feat: live search — update results as you type#75
Open
mthwJsmith wants to merge 2 commits intodddevid:masterfrom
Open
feat: live search — update results as you type#75mthwJsmith wants to merge 2 commits intodddevid:masterfrom
mthwJsmith wants to merge 2 commits intodddevid:masterfrom
Conversation
Adds a "Live Search" setting in Display Settings (default: ON). When enabled, typing in the search box updates the full results view in real time (debounced 300ms) instead of showing the autocomplete dropdown overlay. The overlay behavior is preserved when the setting is turned off.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a live search feature: three new localization keys, a persistent toggle in settings, a PlayerUiSettingsService flag/notifier and persistence, and search screen changes to conditionally perform live searches versus autocomplete based on the setting. Changes
Sequence DiagramsequenceDiagram
actor User
participant SettingsUI as Settings Display UI
participant Service as PlayerUiSettingsService
participant Storage as SharedPreferences
participant SearchScreen as Search Screen
User->>SettingsUI: Toggle Live Search switch
SettingsUI->>Service: call setLiveSearch(value)
Service->>Storage: persist "search_live_search" = value
Service->>Service: liveSearchNotifier.value = value
SearchScreen->>Service: listen/reads liveSearchNotifier
SearchScreen->>SearchScreen: on input debounce -> if liveSearch true run full search else load autocomplete
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/screens/search_screen.dart (1)
41-45: Setting is not reactive to changes made while screen is mounted.The
_liveSearchvalue is read once ininitState(). If a user toggles the setting in the Display tab and returns to this screen without triggering a rebuild, the search behavior won't reflect the change until the widget is recreated.Consider listening to
PlayerUiSettingsService().liveSearchNotifierfor reactive updates:♻️ Suggested improvement for reactive setting updates
class _SearchScreenState extends State<SearchScreen> { final _searchController = TextEditingController(); final _focusNode = FocusNode(); SearchResult? _searchResult; SearchResult? _autocompleteSuggestions; bool _isSearching = false; bool _isLoadingSuggestions = false; bool _showSuggestions = false; String _query = ''; Timer? _debounceTimer; bool _liveSearch = true; + final _uiSettings = PlayerUiSettingsService(); `@override` void initState() { super.initState(); - _liveSearch = PlayerUiSettingsService().getLiveSearch(); + _liveSearch = _uiSettings.getLiveSearch(); + _uiSettings.liveSearchNotifier.addListener(_onLiveSearchChanged); + } + + void _onLiveSearchChanged() { + setState(() { + _liveSearch = _uiSettings.liveSearchNotifier.value; + }); } `@override` void dispose() { _searchController.dispose(); _focusNode.dispose(); _debounceTimer?.cancel(); + _uiSettings.liveSearchNotifier.removeListener(_onLiveSearchChanged); super.dispose(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/screens/search_screen.dart` around lines 41 - 45, The screen reads PlayerUiSettingsService().getLiveSearch() once in initState(), so _liveSearch won't update when the user changes the setting; subscribe to PlayerUiSettingsService().liveSearchNotifier in initState (store the returned Listenable/ValueNotifier subscription if needed), update _liveSearch inside a setState callback when the notifier fires, and remove the listener in dispose to avoid leaks; keep the initial getLiveSearch() for the initial value but rely on the notifier for reactive updates (refer to _liveSearch, initState, dispose, and PlayerUiSettingsService().liveSearchNotifier).
🤖 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/screens/search_screen.dart`:
- Around line 41-45: The screen reads PlayerUiSettingsService().getLiveSearch()
once in initState(), so _liveSearch won't update when the user changes the
setting; subscribe to PlayerUiSettingsService().liveSearchNotifier in initState
(store the returned Listenable/ValueNotifier subscription if needed), update
_liveSearch inside a setState callback when the notifier fires, and remove the
listener in dispose to avoid leaks; keep the initial getLiveSearch() for the
initial value but rely on the notifier for reactive updates (refer to
_liveSearch, initState, dispose, and
PlayerUiSettingsService().liveSearchNotifier).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/l10n/app_en.arblib/screens/search_screen.dartlib/screens/settings_display_tab.dartlib/services/player_ui_settings_service.dart
The setting was only read once in initState(). Now listens to the ValueNotifier so toggling Live Search in Settings takes effect immediately without leaving the search screen.
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
Personally i dislike the popup live search on the search page, and find it superior and matching other popular music apps just to show the live search in the already excellent search page.
I found overlay to be quite a cramped floating card that filled up with unrelated albums for most of my searches (I do use squid.wtf backend which made this feel worse). The full results view imo has much better presentation (I really like it of all the Navidrome clients I used) - horizontal album cards, proper
SongTilerows with controls,ArtistTilerows. Both call the exact same API, so there's no reason not to show the better view live as you type.screen-20260227-030402-1772161431980.mp4
In my opinion, I would just remove the popup, but for this PR I added an options in settings to keep the popup card.
Changes
player_ui_settings_service.dart_keyLiveSearchsetting + getter/setter +ValueNotifiersearch_screen.dartinitState, branch_onSearchChangedto call_search()or_loadAutocomplete()settings_display_tab.dartCupertinoSwitch)app_en.arbTest plan
Summary by CodeRabbit