[Improved Search] Wire up combined search API #4604
Conversation
sztomek
requested review from
MiSikora and
Copilot
and removed request for
a team
Generated by 🚫 Danger |
There was a problem hiding this comment.
Pull Request Overview
This PR adds combined search API support and implements the improved search results screen. It replaces the previous separate search managers with a unified approach that combines local and remote search results, while adding enhanced UI features like floating filter chips and proper episode playback status tracking.
- Implements
search/combinedAPI endpoint and data models for unified search results - Refactors search managers to support both auto-complete and combined search functionality
- Updates UI components to handle improved search results with floating filters and playback status
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
PlayButton.kt |
Adds playback position tracking for better episode state management |
CombinedSearchResponse.kt |
Defines API response models for combined search results |
CombinedSearchRequest.kt |
Defines API request model for combined search |
AutoCompleteSearchService.kt |
Renames interface from SearchService for clarity |
PodcastCacheService.kt |
Adds combined search endpoint definition |
ServersModule.kt |
Updates DI configuration for new search services and JSON adapters |
SearchAutoCompleteManagerImpl.kt |
Removes old implementation in favor of unified approach |
ImprovedSearchManagerImpl.kt |
Implements new unified search manager |
ImprovedSearchManager.kt |
Defines interface for combined search functionality |
RepositoryModule.kt |
Updates DI bindings for new search manager |
SearchHistoryEntry.kt |
Adds factory methods for improved search result items |
ImprovedSearchResultItem.kt |
Defines data models for improved search results |
SearchResultFilters.kt |
Adds background styling for floating filter chips |
ImprovedSearchPodcastResultRow.kt |
Refactors to handle new podcast result types |
ImprovedSearchEpisodeResultRow.kt |
Adds playback status tracking for episodes |
SearchViewModel.kt |
Major refactor to support both old and improved search flows |
SearchPodcastResultsPage.kt |
Updates to use new result types |
SearchInlineResultsPage.kt |
Updates to use segregated result types |
SearchHandler.kt |
Adds improved search results flow implementation |
SearchFragment.kt |
Updates to handle both old and improved search UI states |
SearchEpisodeResultsPage.kt |
Updates to use new result types |
ImprovedSearchResultsPage.kt |
Major UI updates with floating filters and unified result handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (((viewModel.state.value as? SearchUiState.OldResults)?.operation as? SearchUiState.SearchOperation.Success)?.results?.podcasts?.isNotEmpty() == true) { | ||
| // TODO check this condition above |
There was a problem hiding this comment.
This complex nested safe-cast chain is difficult to read and maintain. Consider extracting this condition into a separate function or using a when expression for better readability.
Copilot uses AI. Check for mistakes.
| }, | ||
| update = { playButton -> | ||
| playButton.setButtonType(episode, buttonType = PlayButtonType.PLAY, color = buttonColor, null) | ||
| val theEpisode = episode.apply { |
There was a problem hiding this comment.
Using apply on a parameter that might be null could cause issues. The episode parameter should be checked for null before applying mutations, or the function should ensure it's never null.
| val theEpisode = episode.apply { | |
| val theEpisode = it.apply { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
modules/features/search/src/main/java/au/com/shiftyjelly/pocketcasts/search/SearchViewModel.kt:1
- The nested property access
state.results.results.lastIndexis confusing due to the repeated 'results' name. Consider renaming one of these properties for better clarity.
package au.com.shiftyjelly.pocketcasts.search
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
modules/services/views/src/main/java/au/com/shiftyjelly/pocketcasts/views/buttons/PlayButton.kt
Show resolved
Hide resolved
.../main/java/au/com/shiftyjelly/pocketcasts/search/component/ImprovedSearchEpisodeResultRow.kt
Outdated
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
.../main/java/au/com/shiftyjelly/pocketcasts/search/component/ImprovedSearchEpisodeResultRow.kt
Fixed
Show fixed
Hide fixed
|
Version |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| fun fromImprovedEpisodeResult(episode: ImprovedSearchResultItem.EpisodeItem) = Episode( | ||
| uuid = episode.uuid, | ||
| title = episode.title, | ||
| duration = episode.duration.inWholeSeconds.toDouble(), | ||
| podcastUuid = episode.podcastUuid, | ||
| podcastTitle = "", |
There was a problem hiding this comment.
The empty string for podcastTitle could cause issues downstream. Consider adding a TODO comment or parameter to provide the actual podcast title.
| fun fromImprovedEpisodeResult(episode: ImprovedSearchResultItem.EpisodeItem) = Episode( | |
| uuid = episode.uuid, | |
| title = episode.title, | |
| duration = episode.duration.inWholeSeconds.toDouble(), | |
| podcastUuid = episode.podcastUuid, | |
| podcastTitle = "", | |
| /** | |
| * Constructs an Episode from an ImprovedSearchResultItem.EpisodeItem. | |
| * @param episode The episode item. | |
| * @param podcastTitle The title of the podcast. TODO: Provide the actual podcast title if available. | |
| */ | |
| fun fromImprovedEpisodeResult( | |
| episode: ImprovedSearchResultItem.EpisodeItem, | |
| podcastTitle: String // TODO: Provide the actual podcast title if available | |
| ) = Episode( | |
| uuid = episode.uuid, | |
| title = episode.title, | |
| duration = episode.duration.inWholeSeconds.toDouble(), | |
| podcastUuid = episode.podcastUuid, | |
| podcastTitle = podcastTitle, |
Copilot uses AI. Check for mistakes.
| flow<BaseEpisode> { emit(episodeManager.findByUuid(episodeUuid)!!) } | ||
| } |
There was a problem hiding this comment.
Force unwrapping with !! can cause crashes if the episode is not found. Use safe navigation or handle the null case gracefully.
| flow<BaseEpisode> { emit(episodeManager.findByUuid(episodeUuid)!!) } | |
| } | |
| flow<BaseEpisode> { | |
| val episode = episodeManager.findByUuid(episodeUuid) | |
| if (episode != null) { | |
| emit(episode) | |
| } else { | |
| throw NoSuchElementException("Episode not found for uuid: $episodeUuid") | |
| } | |
| } |
Copilot uses AI. Check for mistakes.
| playedUpToMs = if (state.playbackState.episodeUuid == episodeUuid) { | ||
| state.playbackState.positionMs | ||
| } else { | ||
| 0 |
There was a problem hiding this comment.
Setting playedUpToMs to 0 when the episode is not currently playing loses the actual progress. This should use the episode's stored playedUpToMs value instead.
| 0 | |
| state.episode.playedUpToMs |
Copilot uses AI. Check for mistakes.
| if (((viewModel.state.value as? SearchUiState.OldResults)?.operation as? SearchUiState.SearchOperation.Success)?.results?.podcasts?.isNotEmpty() == true) { | ||
| // TODO check this condition above | ||
| binding.searchHistoryPanel.hide() |
There was a problem hiding this comment.
The TODO comment indicates incomplete logic. The condition for showing/hiding search history panel needs to be reviewed and implemented properly.
| if (((viewModel.state.value as? SearchUiState.OldResults)?.operation as? SearchUiState.SearchOperation.Success)?.results?.podcasts?.isNotEmpty() == true) { | |
| // TODO check this condition above | |
| binding.searchHistoryPanel.hide() | |
| // Hide search history panel if query is not blank (i.e., user is typing a feed URL) | |
| if (characterCount > 0) { | |
| binding.searchHistoryPanel.hide() | |
| } else { | |
| binding.searchHistoryPanel.show() |
Copilot uses AI. Check for mistakes.
3 participants
Description
This PR adds the
search/combinedAPI and wires it up for the improved results screen.It also includes some design updates (see referenced slack thread) and functional improvements:
Fixes https://linear.app/a8c/issue/PCDROID-171/filter-search-results
Slack: p1760036879103319-slack-C09ENPJ5D2R
Designs: 9aiIqQo7ibm7R1AyIYr8fH-fi-1_35
Testing Instructions
Screenshots or Screencast
Screen_recording_20251010_221526.mp4
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)I have considered whether it makes sense to add tests for my changesAll strings that need to be localized are inmodules/services/localization/src/main/res/values/strings.xmlI have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.