Fix SelectPlaylistInput layout and improve maintainability #4156
Conversation
…place dropdown with fixed list to prevent button overlay - Break down into smaller focused components - Add comprehensive test coverage - Reduce spacing for compact layout
Signed-off-by: Deluan <deluan@navidrome.org>
…stDialog.test.jsx
There was a problem hiding this comment.
Hello @deluan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request. The primary goal of this PR is to address a layout issue in the SelectPlaylistInput component where the dropdown overlay was preventing interaction with elements below it. To achieve this, the PR replaces the Material-UI Autocomplete component with a custom implementation using core Material-UI components. This change also serves to significantly improve the maintainability of the component by breaking it down into smaller, more focused sub-components. Additionally, the PR includes updates to internationalization strings and comprehensive test coverage for the refactored component.
Highlights
- Layout Fix: Replaced the Material-UI Autocomplete component with a custom list implementation to resolve an issue where the dropdown overlay blocked interaction with elements positioned below the input.
- Component Refactoring: The monolithic
SelectPlaylistInputcomponent has been broken down into 7 smaller, more manageable functional components, improving code organization and maintainability. - Improved Interaction & UX: The new implementation provides a fixed-height scrollable list for playlists, more compact spacing, and displays selected playlists as chips below the input field for a cleaner UI.
- Internationalization Updates: New translation keys have been added for various UI texts (placeholders, messages, button titles), and the translation structure has been cleaned up.
- Enhanced Testing: 18 new comprehensive tests have been added specifically for the
SelectPlaylistInputcomponent, covering various scenarios including filtering, selection, creation, and edge cases. Existing tests forAddToPlaylistDialogwere updated to reflect the new component's interaction model.
Changelog
Click here to see the changelog
- resources/i18n/pt-br.json
- Added new translation keys for playlist search placeholder (
searchOrCreate), create new hint (pressEnterToCreate), remove from selection title (removeFromSelection), and remove symbol (removeSymbol). - Added new translation keys for empty states (
noPlaylistsFound,noPlaylists).
- Added new translation keys for playlist search placeholder (
- ui/src/dialogs/AddToPlaylistDialog.test.jsx
- Updated tests to simulate clicking on playlist list items for selection instead of using keyboard navigation (
ArrowDown,Enter). - Updated tests to simulate pressing Enter in the search box to create a new playlist, removing previous keyboard navigation steps.
- Updated tests to simulate clicking on playlist list items for selection instead of using keyboard navigation (
- ui/src/dialogs/SelectPlaylistInput.jsx
- Removed Material-UI
Autocompletecomponent. - Replaced
Autocompletefunctionality with a custom implementation using Material-UIList,ListItem,TextField,Box, etc. - Refactored the component into 7 smaller, dedicated components:
PlaylistSearchField,EmptyPlaylistMessage,PlaylistListItem,CreatePlaylistItem,PlaylistList,SelectedPlaylistChip, andSelectedPlaylistsDisplay. - Implemented state management for search text and selected playlists.
- Added logic for filtering playlists based on search input.
- Implemented playlist selection/deselection via clicking list items.
- Added functionality to create new playlists by typing in the search box and pressing Enter or clicking the Add icon.
- Added display of selected playlists as interactive chips below the list.
- Added PropTypes for all new sub-components.
- Removed Material-UI
- ui/src/i18n/en.json
- Added new English translation keys mirroring the changes in
pt-br.jsonfor search placeholder, create new hint, remove button text/symbol, and empty state messages.
- Added new English translation keys mirroring the changes in
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A component, once large,
Now broken down with care,
Easier to test.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly refactors the SelectPlaylistInput component, improving maintainability, fixing a key UI bug, and enhancing the user experience. The breakdown into smaller, focused components is very well done, and the addition of comprehensive tests is highly commendable. The internationalization updates are also thorough.
I have one point of clarification regarding the logic for allowing the creation of a new playlist, detailed below. Overall, fantastic work!
Summary of Findings
- Component Refactoring: The refactoring of
SelectPlaylistInputinto multiple smaller, focused components is a significant improvement for code clarity, maintainability, and testability. This is very well executed. - Testing: The test suite for the new
SelectPlaylistInputcomponent is comprehensive, covering a wide range of scenarios including basic functionality, selection, creation, edge cases, and keyboard navigation. The tests are well-structured and clear. - UI Bug Fix: The change from Material-UI Autocomplete to a custom fixed list successfully addresses the reported dropdown overlay issue, improving UX.
- Internationalization: New translation keys have been added and existing translations updated, ensuring the new UI elements are localized.
- Playlist Creation Logic: A review comment was added to discuss the
canCreateNewlogic, specifically how it determines if a typed playlist name already exists by checking againstfilteredOptions. While likely correct for the UI context, it's worth confirming this aligns with overall duplicate prevention strategy.
Merge Readiness
This pull request is of high quality and introduces substantial improvements. The core objectives of fixing the UI layout and improving maintainability have been successfully met. The testing is thorough.
There is one medium severity comment regarding the canCreateNew logic for clarification. Depending on the outcome of that discussion, a minor adjustment might be considered, or the current logic might be confirmed as sufficient.
I am unable to approve pull requests directly. However, once the point in the review comment is addressed or clarified, this PR should be in excellent shape for merging after further review and approval from authorized team members.
| !filteredOptions.some( | ||
| (option) => | ||
| option.name.toLowerCase() === searchText.toLowerCase().trim(), | ||
| ) && |
There was a problem hiding this comment.
The canCreateNew logic currently checks if a playlist name conflicts with filteredOptions (i.e., playlists visible after applying the current search text) and selectedPlaylists.
!filteredOptions.some(
(option) =>
option.name.toLowerCase() === searchText.toLowerCase().trim(),
)Could you clarify if this is the intended behavior? If a user types a playlist name that exists in their full list of writable playlists (options) but is not currently visible due to the search filter, this logic would allow them to proceed with creating a new playlist with that (globally existing) name.
For example:
- User's playlists:
["Rock Classics", "Pop Hits", "Jazz Vibes"] - User types search text:
"pop"->filteredOptionsbecomes["Pop Hits"] - User then clears search and types:
"Rock Classics"for a new playlist. searchTextis now"Rock Classics".filteredOptions(based on the newsearchText="Rock Classics") would be["Rock Classics"].- The condition
!filteredOptions.some(...)would correctly evaluate tofalse, preventing creation.
However, consider this scenario:
- User's playlists:
["Rock Classics", "Pop Hits", "Jazz Vibes"] - User types search text:
"Jazz"->filteredOptionsbecomes["Jazz Vibes"] - User then changes their mind and types in the search box:
"Pop Hits"(intending to create a new one, perhaps forgetting it exists or it's not visible). searchTextis now"Pop Hits".filteredOptions(based onsearchText="Pop Hits") would be["Pop Hits"].- The condition
!filteredOptions.some(o => o.name.toLowerCase() === "pop hits")would befalse, correctly preventing creation of an exact match to a visible item.
My apologies, re-evaluating the logic: filteredOptions is re-calculated based on the current searchText. So, if the user types an existing name, filteredOptions will contain that existing playlist (if it matches the search term, which it would if typed fully). The current logic correctly prevents creating a new playlist if its name exactly matches an item in the filteredOptions.
However, the canCreateNew flag is also used to determine if the "Create new playlist" option appears in the list (CreatePlaylistItem) and if the Add icon appears in the search field. The condition filteredOptions.length > 0 in PlaylistList (line 195) means CreatePlaylistItem only shows if there are other filtered results. If the user types a unique name, filteredOptions might be empty, and then CreatePlaylistItem wouldn't show, but the Add icon in PlaylistSearchField would (due to canCreateNew being true), and Enter key would also work. This seems consistent.
Perhaps the only subtle point is if a playlist My Playlist exists, and the user types my playlist (different case). The current check option.name.toLowerCase() === searchText.toLowerCase().trim() handles this correctly by preventing creation.
Given this re-evaluation, the current logic seems robust for preventing duplicates based on what's effectively visible or searchable. The main concern would be if options (all writable playlists) should be the source of truth for the existence check, rather than filteredOptions. However, since filteredOptions is derived from options and reflects the current search, it seems appropriate for the UI context.
No change might be needed, but wanted to ensure this interaction is fully considered. If the backend handles unique playlist name constraints (case-insensitively, perhaps per user), then this client-side check is a good UX enhancement.
Summary
This PR fixes the SelectPlaylistInput dropdown overlay issue and significantly improves code maintainability through component refactoring.
Key Changes
🐛 Layout Fix
🏗️ Component Refactoring
Broke down the monolithic component into 7 smaller, focused components:
✅ Testing & Quality
Impact
Closes the issue where SelectPlaylistInput dropdown overlay prevented clicking action buttons below the component: #1919