[TV] Search suggestions and history by sztomek · Pull Request #5746 · Automattic/pocket-casts-android

sztomek · 2026-08-14T15:33:35Z

Description

Brings the Android TV Search screen up to parity with the Apple TV app (pocket-casts-ios/Pocket Casts TV App/UI/Search), closing a set of gaps found in a behavioural audit. Stacked on #5745 (the combined-search crash fix) — review/merge that first; this PR's diff excludes it.

What changed

Area	Before (Android TV)	After (Apple TV parity)
Debounce	2000ms (shared phone remote-config default)	300ms TV constant — matches iOS, far snappier
Empty results	generic "No results"	"No results for "term"" (shows the query)
Loading	bare spinner	spinner + "Searching…" label (string existed, was unused)
Scope on clear	reset to Top Results	preserved (matches iOS)
Field prompt	"Search"	"Podcasts, shows, authors"
Search flow	single `combinedSearch` (all-at-once)	two-phase: predictive (`autoCompleteSearch`) → fast podcasts + term suggestions, then full search fills in episodes
Suggestions	none	term suggestions while typing, rendered above the keyboard; tap to fill
History	never saved/shown (manager only used to wipe on sign-out)	saved on finishing a search / picking a suggestion + "Recent searches" row in the idle area
Video episodes	`has_video` dropped by the DTO	parsed through DTO → model, driving a Featured video row
Top Results	Podcasts → Episodes (vertical)	Featured (video) → Episodes → Podcasts horizontal carousels (matches `SearchTopResultsView`)
Podcast ordering	subscribed-first	server-relevance first, subscribed fill gaps (matches iOS) — see flag below

Shared servers/model change: CombinedResult.EpisodeResult now parses has_video, and ImprovedSearchResultItem.EpisodeItem carries hasVideo (additive, defaults false — benefits phone too).

⚠️ Needs design approval

There is an ongoing conversation about the UX, see slack: p1786735532787109-slack-C0ATWH7BNH3

UPDATE
UX is approved!

Fixes POC-848 https://linear.app/a8c/issue/POC-848/search-suggestions-and-history

Notes / follow-ups

Suggestions can't live inside the system keyboard — Android exposes no API to inject candidates into a third-party IME (and a custom keyboard was previously rejected by design), so they render in-app just above the keyboard.
Episodes scope still shows a flat grid of all episodes; splitting out the Featured video row there too is a small follow-up.
Focus-restore after returning from a detail now lands on the first Top-Results section (was the Podcasts row) — coarse, not a bug.
Device UI verification pending: the shared TV emulator is hard to drive precisely and isn't logged in; the logic + crash fix are unit-tested and all module builds/tests are green, but the interactive Compose UI (suggestions/history/carousels focus) should be eyeballed on a real device. Screenshots to follow.

Testing Instructions

Merge/stack on [TV] Fix search failing on unknown combined-search result types #5745 so search works (otherwise network-type results still error).
./gradlew :tv:installDebug. Open Search, type a query:
- term suggestions appear above the keyboard while typing; tapping one fills + searches.
- podcasts appear quickly, then episodes fill in.
- Top Results shows Featured (video) → Episodes → Podcasts carousels.
- an empty query returns to the idle browse; if you've searched before, a Recent searches row appears.
- a no-match query shows "No results for "…"".
Unit tests: ./gradlew :tv:testDebugUnitTest :modules:services:servers:testDebugUnitTest :modules:services:repositories:testDebugUnitTest.

Screenshots or Screencast

Screen_recording_20260814_212101.mp4

Checklist

If this is a user-facing change, I have added an entry in CHANGELOG.md — N/A (TV, pre-release)
Ensure the linter passes (./gradlew spotlessApply)
I have considered whether it makes sense to add tests for my changes
All strings that need to be localized are in localization
Any jetpack compose components I added or changed are covered by compose previews
I have updated (or requested that someone edit) the Event Horizon schema — N/A (no analytics change)

claude · 2026-08-14T15:33:52Z

Claude finished @sztomek's task in 6m 3s —— View job

Review: [TV] Search parity with Apple TV

Gather context (diff vs fix/tv-search-network-crash, read changed files)
Review ViewModel / search flow logic
Review Compose UI changes
Review shared servers / model / repositories changes
Post review (6 inline comments + summary below)

Nice piece of work — the parity table in the description made this easy to follow, and calling out the podcast-ordering reversal for a decision rather than burying it is exactly right. The hasVideo plumbing is minimal and additive, the debounce constant is a clear win, and the test diff tracks the behaviour changes honestly (renaming "subscribed podcasts lead the results" rather than deleting it).

Most of what follows falls out of one thing: the search is now two-phase, and a few places still assume it's atomic.

Blocking-ish

1. Unguarded suspend calls can crash the app — TvSearchViewModel.kt:122
findSubscribedFlow(term).first() moved out of the try during the refactor. viewModelScope has no exception handler, so a throw is uncaught rather than TvSearchState.Error. Same for refreshHistory() in init (L106) and searchHistoryManager.add(...) in saveSearchTerm (L169). Every other suspend call in the file is wrapped, so this looks accidental. Fix this →

2. Transient false "No results for …" — TvSearchViewModel.kt:135
The early publish hard-codes episodes = emptyList(). Because the scope is no longer reset on clear, a user parked on the Episodes filter gets the full no-results empty state mid-search, then a populated grid a round-trip later. Suggest an isPartial flag on Results that the empty branches treat as "still loading". Details inline.

3. The two phases are serialised — TvSearchViewModel.kt:122-139
autoCompleteSearch is fully awaited before combinedSearch is issued, so episodes now arrive one extra RTT later than on the base branch. Overlapping them with async preserves the early-podcast win without regressing full-result latency.

Worth fixing before merge

4. Three dead imports in TvSearchEpisodeRow.kt after deleting the TvSearchEpisodeRow composable — FocusRequester (L21), focusRequester (L22), TvEpisodeListItemContainer (L33). None are referenced any more. Fix this →

5. Preview coverage — the checklist claims previews cover the new components, but TvSearchScreenPreview still only renders Idle with an empty history. Nothing exercises the suggestions list, the Recent-searches row, the Featured carousel, or the new TvSearchLoading label — which is a shame given the interactive bits are the ones you can't verify on the emulator. Fix this →

6. Redundant truncation — SearchHistoryManagerImpl.add() already truncates to 20, so truncateHistory(SEARCH_HISTORY_LIMIT) and the constant duplicate repository behaviour, and the new test asserts on the duplicate. Inline.

7. isFollowed is now wrong for predictive podcasts — ImprovedSearchManagerImpl.autoCompleteSearch never populates isSubscribed, and predictive entries now win the distinctBy, dropping the correctly-flagged local copy. Invisible today (TvPodcastTile ignores the field) but it's latent. Inline.

Focus / UX to verify on device

8. Focus black hole after picking a suggestion — the focused tile is removed from composition in the same frame with no explicit focus target. Other restore sites here all use a FocusRequester; this one doesn't. Inline.

9. Rows inserted above the focus — when the full search lands, Featured and Episodes carousels appear above the podcasts row the user may already be scrolling. That's the price of progressive + Apple's row order; just flagging it as the thing to watch when you get a device.

On your open question — podcast ordering

Worth noting that the code doesn't quite do what the description says. The precedence is predictivePodcasts → remotePodcasts → localPodcasts, so the visible order is dominated by the autocomplete endpoint's relevance, not combinedSearch's. That's arguably the better choice — the row doesn't reshuffle when the full results land — but it isn't "server-relevance first" in the sense the table implies, so it's worth stating explicitly when the team decides.

For what it's worth: subscribed-first is a stronger default on a 10-foot UI, where scrolling is expensive and the shows you follow are the ones you're most likely reaching for. If you keep remote-first, a "Your shows" section would recover most of that.

Smaller notes

hasVideo: Boolean = false with Moshi codegen handles an absent key fine, but an explicit "has_video": null will throw. Boolean? + ?: false is cheap insurance if the server's contract isn't guaranteed.
Suggestions are only cleared on an empty query, so re-entering the field after a completed search briefly shows stale suggestions for the previous term.
Saving on field blur persists abandoned and typo'd terms — and this writes into the shared search_history table that the phone app reads, so TV noise shows up in phone recents. (The term column is uniquely indexed, so the key = { it } on the Recent searches row is safe — I checked.)
findAll(showFolders = false) loads podcast and episode history rows just to filterIsInstance them away.
TvSearchLoading has no spacing between the spinner and the "Searching…" label.

I did not build or run the test suite in this environment; findings are from reading the diff and surrounding code.
• branch feat/tv-search-parity

dangermattic · 2026-08-14T15:33:56Z

	1 Warning
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

wpmobilebot · 2026-08-17T11:26:53Z

Version 8.19 has now entered code-freeze, so the milestone of this PR has been updated to 8.20.

claude · 2026-08-24T12:52:14Z

Claude finished @sztomek's task in 4m 36s —— View job

Review: [TV] Search suggestions and history

Gather context (diff vs origin/main, read changed files)
Re-check previously raised findings against the new commits
Review ViewModel / search flow logic
Review Compose UI changes
Review shared servers / model / repositories changes
Post review (4 inline comments + summary below)

Re-reviewed at 21d3ea9. The follow-up commits closed almost everything from the previous pass, and closed it properly rather than minimally:

Previous finding	Status
Unguarded suspend calls	✅ `findSubscribedFlow` back inside the `try`; `init` and `saveSearchTerm` wrapped with the `CancellationException` rethrow — plus a regression test
Transient false "No results"	✅ `isPartial` on `Results`, both empty branches fall back to `TvSearchLoading()`
Serialised phases	✅ `async { runCatching { combinedSearch(term) } }` overlaps the two round-trips
Dead imports	✅ removed
Preview coverage	✅ four new previews — history, results with a video episode, suggestions, loading
Redundant `truncateHistory`	✅ dropped, manager owns its cap
`isFollowed` wrong for predictive podcasts	⚠️ fixed for the final list (`:154`), missed on the early publish — inline
Focus black hole	❌ still unaddressed — inline, and it's now reachable from two paths

Nothing here is a merge blocker on its own; the two focus items are the ones I'd want eyes on before this ships.

Worth fixing

1. Moving down into the suggestions row saves the half-typed term — TvSearchScreen.kt:230-235
TvSearchField treats any focus loss as "finished editing", and its own onPreviewKeyEvent handler calls moveFocus(Down) — so the headline interaction (type sug → D-pad down → pick "sugar rush") persists both sug and sugar rush. Because search_history is shared with the phone app, abandoned prefixes typed on the TV surface in phone recents. Details and a suggested shape inline.

2. Focus is dropped when a click removes the focused tile — TvSearchScreen.kt:314-321
Tapping a Recent search takes the state Idle → Searching, which replaces the focused TvTile with TvSearchLoading() — a composable with no focusable content whatsoever, and no FocusRequester pointing anywhere. The suggestions row has the same shape when a debounce cycle returns no Terms. Every other focus-restore site in this file uses an explicit requester; these two don't. Top of the device-verification list.

3. Early publish skips the isFollowed correction — TvSearchViewModel.kt:145-148
localUuids is computed one line above but only applied to the final list, so followed podcasts are published isFollowed = false and flip a round-trip later. Latent (nothing renders it on TV yet), one-line fix, suggestion inline.

4. Request volume — TvSearchViewModel.kt:128-133
Debounce 2000ms → 300ms and a second endpoint per cycle ≈ 13× the search requests of the base branch per typing session. Matches Apple TV so it's probably intended, but it's a backend-load change riding inside a UI-parity PR and deserves an explicit nod rather than being inferred from a table row.

I posted a follow-up on that thread retracting a secondary suggestion in it — the runCatching inside the async is load-bearing (a failing async child cancels the parent launch regardless of where await() sits, so without it a combinedSearch failure reaches viewModelScope uncaught). Leave that as written.

Test coverage

The new tests are good and honest — a failure fetching subscribed podcasts surfaces the error state directly locks in the fix for the previous round's finding #1, and the renames (clearing the query preserves the selected filter, server results lead and subscribed podcasts fill the remaining gaps) document the behaviour reversals instead of hiding them.

The gap is the progressive publish itself, which is the centrepiece of the PR and has no test. A Turbine assertion that searchState emits Results(podcasts = […], episodes = [], isPartial = true) before the full Results would cover both the early-publish ordering and the isPartial contract that TvSearchResults now depends on — and would have caught finding #3 above.

On the open design question

For the record, the code still doesn't match what the description says. The precedence is predictivePodcasts → remotePodcasts → localPodcasts, so visible order is dominated by the autocomplete endpoint, not combinedSearch. That's arguably the better behaviour (the row doesn't reshuffle when full results land), but it isn't "server-relevance first" in the sense the table implies. Worth saying out loud when the team decides, since the Slack thread is still open.

My earlier view hasn't changed: subscribed-first is a stronger default on a 10-foot UI, where scrolling is expensive and the shows you follow are the ones you're most likely reaching for. If remote-first wins, a "Your shows" section recovers most of it.

Smaller notes

showSuggestions toggling inserts/removes a ~72dp block (row + spacer) above the filter chips and results, so the whole column shifts every time suggestions appear or disappear mid-typing. Add to the device-verification list.
TvSearchSuggestions uses items(suggestions) with no key, so after picking a suggestion the tile under focus keeps its slot but silently changes term. The history row two hundred lines up does pass key = { it }. Either is defensible — worth being deliberate about which.
featured / otherEpisodes / topPodcasts in TvSearchTopResults:497-499 are recomputed on every recomposition; remember(episodes) would be tidier, though the lists are small.
tv_search_no_results_for_title escapes the curly quotes (\“…\”) — Android drops the backslash before a non-special char, so it renders correctly, but the escapes are noise.
refreshHistory() calls findAll(showFolders = false), loading podcast and episode history rows just to filterIsInstance them away.
hasVideo: Boolean? = null + it.hasVideo == true — good, that's the null-safe form.

I could not run ./gradlew :tv:testDebugUnitTest or spotlessCheck in this environment (the Gradle invocation isn't in the allowed tool list), so findings are from reading the diff and surrounding code only.
• branch feat/tv-search-parity

claude · 2026-08-24T13:12:58Z

Claude finished @sztomek's task in 2m 29s —— View job

Review: [TV] Search suggestions and history

Gather context (diff vs origin/main, new commits since last review)
Re-check previously raised findings
Review ViewModel / search flow logic
Review Compose UI changes
Post review (1 new inline comment + summary below)

Re-reviewed at 8f428ad. The only change since the last pass is the one-line isFollowed fix on the early publish, which closes finding #3 exactly as suggested:

val earlyPodcasts = (predictivePodcasts + localPodcasts)
    .distinctBy(ImprovedSearchResultItem.PodcastItem::uuid)
    .map { if (it.uuid in localUuids) it.copy(isFollowed = true) else it }

Status of the previous round:

Finding	Status
#1 Half-typed term saved on blur	❌ open — thread
#2 Focus dropped when a click removes the focused tile	❌ open — thread
#3 Early publish skips the `isFollowed` correction	✅ fixed in `8f428ad`
#4 Request volume	ℹ️ acknowledged, no change expected

One new finding

The 300ms debounce turns the Searching state into a strobe — TvSearchViewModel.kt:130

_searchState.value = TvSearchState.Searching unconditionally discards whatever is on screen, and TvSearchContent maps it to a full-screen TvSearchLoading(). The line is unchanged from main, but the debounce is not: at 2000ms it fired roughly once per typing session; at 300ms it fires on nearly every keystroke pause, so typing business blanks the results area and re-renders the spinner ~7 times before settling.

It's the same class of problem isPartial was added to solve — don't show a terminal state while data is in flight — except the Searching branch sits above isPartial and still wins. Gating it on _searchState.value !is TvSearchState.Results keeps refined queries stable and only clears on a cold start. Details and a sketch in the inline comment. Fix this →

This also compounds finding #2: TvSearchLoading() has no focusable content, so every entry into Searching destroys the focus owner. Harmless while the user is typing (focus lives in the field), but it is the mechanism behind the recent-search-tap dead-remote case. Worth fixing the two together.

Still open from last round

Half-typed terms reach shared history. TvSearchField treats any focus loss as "finished editing", and its own onPreviewKeyEvent calls moveFocus(Down) — so the headline interaction (type sug → D-pad down → pick "sugar rush") persists both sug and sugar rush. search_history is shared with the phone app, so abandoned TV prefixes surface in phone recents. The ImeAction.Search and Back paths already distinguish a deliberate completion from a plain focus move; plumbing an onSearchSubmitted out of keyboardActions.onSearch would keep history to committed terms.

Note that the new test searching does not save partial terms to history only covers the ViewModel, which never had this behaviour — the persistence decision lives in TvSearchScreen.kt:231, and that path is untested.

Focus black hole on recent-search tap. onHistorySelect is viewModel::onQueryChange, so ~300ms after the click TvSearchIdle (holding the focused TvTile) is swapped for TvSearchLoading(). Every other focus-restore site in this file uses an explicit FocusRequester (TvSearchTopResults:487, TvSearchEpisodeGrid:574, TvSearchDiscover:354); this one and the suggestions row do not. Top of the device-verification list.

Test coverage

The new tests are well chosen and honest — a failure fetching subscribed podcasts surfaces the error state locks in the previous round's crash fix, and the renames (clearing the query preserves the selected filter, server results lead and subscribed podcasts fill the remaining gaps) document the behaviour reversals rather than hiding them.

Still no test for the progressive publish, which is the centrepiece of the PR. A Turbine assertion that searchState emits Results(podcasts = […], episodes = [], isPartial = true) before the final Results would cover both the early-publish ordering and the isPartial contract TvSearchResults now depends on — and would have caught the isFollowed gap that took an extra round to close.

Smaller notes (unchanged, all optional)

TvSearchSuggestions uses items(suggestions) with no key, while the history row at :319 passes key = { it }. If a debounce cycle returns a shorter list, the focused index can disappear.
showSuggestions inserts/removes a ~72dp block above the filter chips, so the whole column shifts as suggestions appear and disappear mid-typing.
featured / otherEpisodes / topPodcasts at TvSearchTopResults:497-499 recompute on every recomposition; remember(episodes) would be tidier.
tv_search_no_results_for_title escapes the curly quotes (\“…\”) — renders fine, but the backslashes are noise.
refreshHistory() calls findAll(showFolders = false), loading podcast and episode rows just to filterIsInstance them away.

On the open design question

Unchanged from last round, and worth restating since the Slack thread is still open: the code's precedence is predictivePodcasts → remotePodcasts → localPodcasts, so visible order is dominated by the autocomplete endpoint, not combinedSearch. That's defensible (the row doesn't reshuffle when full results land) but it isn't "server-relevance first" in the sense the description's table implies. My view is still that subscribed-first is the stronger default on a 10-foot UI; if remote-first wins, a "Your shows" section recovers most of it.

I could not run ./gradlew :tv:testDebugUnitTest or spotlessCheck in this environment (the Gradle invocation isn't in the allowed tool list), so findings are from reading the diff and surrounding code only.
• branch feat/tv-search-parity