[TV] Search: combined search results (podcasts + episodes) - #5728
Conversation
Generated by 🚫 Danger |
8cec375 to
d77dad0
Compare
|
@copilot please review this |
|
@claude please review this |
|
Claude finished @sztomek's task in 4m 35s —— View job Code reviewRead the full diff against A handful of things worth addressing before merge, then some nits. 1.
|
geekygecko
left a comment
There was a problem hiding this comment.
Looks great. Only a couple of minor tweaks, which could probably come in a later PR. When you press Enter or Search on the keyboard, it might be nice for the keyboard to disappear.
Also, when you open the three dot menu on an episode row, it can sometimes take a little while to load, so a loading indicator there might be helpful too.
425f5e8 to
a1420f6
Compare
Co-authored-by: sztomek <33399753+sztomek@users.noreply.github.com>
7bc70d4 to
17d971b
Compare
|
Claude finished @sztomek's task in 2m 59s —— View job Re-review after
|
| Previous finding | Status |
|---|---|
Unused tv_search_searching string |
✅ Removed |
TvSearchEpisodeRow duplicated TvEpisodeListItem's wrapper |
✅ TvEpisodeListItemContainer extracted; both call sites use it, single MORE_BUTTON_ANIMATION_DURATION_MS |
| Error state reused the no-results copy, no retry | ✅ New tv_search_error_subtitle + Retry via TvEmptyState |
| Focus restore broke for episodes-only results | ✅ restoreFocusRequester now falls back to the first episode row |
TvSearchSectionTitle re-implemented TvRow's title |
✅ TvSectionTitle hoisted out of TvRow and shared |
| Episodes not de-duplicated by uuid | ✅ distinctBy added + a test |
| Nothing exercised cancel-on-keystroke | ✅ New debounce-window test |
A few things left, all minor — none of them blockers on top of @geekygecko's approval.
1. The three-dot menu has no in-flight state, and no guard against repeat presses
This is the concrete version of @geekygecko's "it can sometimes take a little while to load". TvSearchViewModel.openEpisodeActions (tv/.../search/TvSearchViewModel.kt:146-163) awaits hydrate() — which can hit findOrDownloadPodcastRxSingle, a full network round trip — before _actionsEpisode is set. Until it resolves, the UI is unchanged, so the press looks like it did nothing.
Two consequences worth handling together:
- No feedback during the wait (the review comment).
- No in-flight guard: each press launches a fresh coroutine. Mashing the "…" button (easy on a remote when nothing visibly happened) stacks N concurrent
findOrDownloadPodcastRxSinglecalls. Same forplayEpisodeat:126— repeated Enter on a row queues multipleplayNowSuspendcalls.
A single MutableStateFlow<String?> of the in-flight episode uuid would drive a spinner and let both functions early-return while one is pending.
2. Retry pays the debounce, then drops focus
TvSearchScreen.kt:190 wires Retry to onQueryChange(query). That does re-run the search (the job is imperative, not a StateFlow collect, so re-setting the same query is not swallowed) — but it re-enters delay(settings.getPodcastSearchDebounceMs()) first, so Retry sits visibly idle for the debounce before anything happens.
Then when the state flips to Searching, LoadingView replaces the TvEmptyState and the currently-focused Retry button is disposed with nothing focusable to take its place. Focus falls to the root; the user recovers with the next D-pad press (re-entering the focus group hits searchFieldFocusRequester), but it's a stumble. A viewModel.retry() that skips the debounce and reuses the existing search body would fix both halves.
3. Focus restore still lands somewhere other than where the user was
TvSearchScreen.kt:261-299 now has a valid target in both branches, so the crash-and-swallow path is gone. Two rough edges remain:
- Coming back from an episode row's Go to podcast, focus lands on the podcasts carousel (or episode index 0), not the episode row that opened the overlay.
- In the episodes-only branch the requester is on
index == 0. If the user scrolled far enough that item 0 left theLazyColumn's composed window,requestFocus()throws andrunCatchingswallows it — nothing focused. And when it does succeed it yanks the list back to the top.
Tracking the uuid that opened the overlay and restoring to that row's requester covers both.
4. The Episodes header no longer animates with focus
Sharing TvSectionTitle fixed the style drift, but TvRow still animates its title 17f → 21f on focus (TvRow.kt:77-80) while the Episodes header is pinned at the default. Moving focus from the podcasts carousel down into the episodes list shrinks the Podcasts title and grows nothing — the two headers read as different kinds of heading. Fine if that's the iOS design; otherwise a focusGroup + animateFloatAsState around the episodes section would match.
Also: .padding(contentPadding).padding(bottom = 17.dp) is now spelled out at both call sites (TvRow.kt:90-92, TvSearchScreen.kt:285-287). Baking the 17.dp bottom padding into TvSectionTitle would stop that one drifting.
Nits
content: @Composable (Modifier) -> Unit(TvEpisodeListItem.kt:59) — passing aModifierdown through a slot works, butcontent: @Composable RowScope.() -> Unitis the more idiomatic shape: callers writeModifier.weight(1f)themselves and the container doesn't have to guess what the child needs. Not worth churn if you prefer the explicitness.- D-pad Down is consumed unconditionally.
TvSearchField.kt:113-116callsfocusManager.moveFocus(FocusDirection.Down)and returnstrueregardless of the result, so inSearchingandNoResults— where nothing below is focusable — the keypress is swallowed. Pre-existing (the file comes from [TV] Load auth-specific discover feeds on TV home #5722, untouched here), but this PR is what makes those states reachable. Returning themoveFocusresult lets the event fall through. (Correcting my earlier review, which described this as already returning the result — it doesn't.) - Test gaps.
hydrate()'s three-step fallback and theplayStarted/playFailuresemissions are still untested. The new dedupe and debounce tests cover the parts I flagged as most fragile, so this is a lower priority than it was. isFollowed/publishedDateare still carried but never rendered — unchanged from last round, just noting the data is unused.- Duplication with
TvHomeViewModel(playEpisode+playStarted/playFailures+hydrate) — still your stated follow-up, and the in-flight guard in point 1 would be a third thing to keep in sync. Might be the nudge to extract it now.
I couldn't run Gradle in this environment (./gradlew is blocked by the sandbox), so spotlessCheck and :tv:testDebugUnitTest are unverified — the tests are reviewed by reading only. One thing I did check statically: thenReturn(300) against getPodcastSearchDebounceMs(): Long is fine, Kotlin types the literal as Long from the expected type.
• branch feat/tv-search-results
Description
Wires real search into the Android TV Search screen, matching the Apple TV app. Until now the TV Search tab only rendered the idle Discover browse (categories + featured rows) with a display‑only text field. This PR makes it actually search and adds the results UI + podcast/episode handling.
Mirrors the iOS
Pocket Casts TV App/UI/Searchflow:ImprovedSearchManager.combinedSearch(the same combined endpoint iOS uses) and merges locally‑subscribed podcasts ahead of the server results, de‑duped by uuid.TvRowof covers) + an Episodes list (podcast name · title · duration, a port of iOSSearchEpisodeRow).TvPodcastDetailsScreenin aTvDetailOverlay; tapping an episode hydrates + plays it and opens Now Playing (the exactTvHomeViewModel.playEpisodeflow,SourceView.SEARCH_RESULTS). Focused episode rows reveal a "…" more button (as in Up Next / podcast details) that opens the sharedTvEpisodeActionsModal— Play, Episode details, Go to podcast, Play next/last, Mark as played, Archive. The DTO is hydrated on open so every action persists.The only shared‑module change:
ImprovedSearchResultItem.EpisodeItemnow carriespodcastTitle(the server already returnspodcast_title; it was being dropped), so the episode rows can show the podcast name like iOS. Additive, phone code unaffected.Follow‑up (noted, not done here):
playEpisode+ itsplayStarted/playFailuresplumbing is now duplicated between the Home and Search view models/screens. Worth extracting into a shared holder before a third tab copies it.Stacked on
feat/tv-home-auth-feeds(#5722).Fixes POC-800 https://linear.app/a8c/issue/POC-800/wire-up-search-apis
Testing Instructions
science). After a short debounce, results appear: a Podcasts carousel and an Episodes list.Screenshots or Screencast
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xml