Skip to content

[TV] Search: combined search results (podcasts + episodes) - #5728

Merged
sztomek merged 7 commits into
mainfrom
feat/tv-search-results
Aug 14, 2026
Merged

[TV] Search: combined search results (podcasts + episodes)#5728
sztomek merged 7 commits into
mainfrom
feat/tv-search-results

Conversation

@sztomek

@sztomek sztomek commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

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/Search flow:

  • Debounced combined search (cancel‑on‑keystroke). Reuses ImprovedSearchManager.combinedSearch (the same combined endpoint iOS uses) and merges locally‑subscribed podcasts ahead of the server results, de‑duped by uuid.
  • Results — a Podcasts carousel (TvRow of covers) + an Episodes list (podcast name · title · duration, a port of iOS SearchEpisodeRow).
  • Actions — tapping a podcast opens the existing TvPodcastDetailsScreen in a TvDetailOverlay; tapping an episode hydrates + plays it and opens Now Playing (the exact TvHomeViewModel.playEpisode flow, SourceView.SEARCH_RESULTS). Focused episode rows reveal a "…" more button (as in Up Next / podcast details) that opens the shared TvEpisodeActionsModal — Play, Episode details, Go to podcast, Play next/last, Mark as played, Archive. The DTO is hydrated on open so every action persists.
  • Idle (blank query) keeps the existing Discover browse.

The only shared‑module change: ImprovedSearchResultItem.EpisodeItem now carries podcastTitle (the server already returns podcast_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 + its playStarted/playFailures plumbing 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

  1. Run the TV app on an Android TV emulator/device and open the Search tab.
  2. The idle screen shows the search field (system on‑screen keyboard auto‑opens), Browse categories, and the Discover rows.
  3. Type a query (e.g. science). After a short debounce, results appear: a Podcasts carousel and an Episodes list.
  4. Press D‑pad down from the field into the results.
  5. Focus an episode row → the "…" button appears; open it → the episode actions dialog. Try Episode details (opens show notes), Go to podcast, Play, etc.
  6. Select a podcast → its details open in the overlay. Select an episode → it starts playing and Now Playing opens.
  7. Clear the query → the idle Discover browse returns.

Screenshots or Screencast

Screenshot_20260812_140105

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • 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 to reflect any new or changed analytics.

@dangermattic

dangermattic commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator
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

@sztomek
sztomek force-pushed the feat/tv-search-results branch from 8cec375 to d77dad0 Compare August 12, 2026 10:53
@sztomek sztomek changed the title [TV] Search: combined search results with Podcasts/Episodes filter [TV] Search: combined search results (podcasts + episodes) Aug 12, 2026
@sztomek sztomek added this to the 8.19 milestone Aug 12, 2026
@sztomek
sztomek marked this pull request as ready for review August 12, 2026 12:04
@sztomek
sztomek requested a review from a team as a code owner August 12, 2026 12:04
@sztomek
sztomek requested review from geekygecko and removed request for a team August 12, 2026 12:04
@sztomek

sztomek commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please review this

Copilot AI commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

@copilot please review this

Addressed an issue I found in 7bc70d4: the TV search episode actions modal now uses a dedicated Search Results action context so its analytics source matches the screen, and I added a focused unit test for it.

@sztomek

sztomek commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

@claude please review this

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

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


Code review

Read the full diff against origin/feat/tv-home-auth-feeds plus the surrounding TV components. Overall this is clean, idiomatic work — cancel-on-keystroke debounce, CancellationException rethrown in every catch, local-first dedupe, previews on both new composables, and tests for the view-model states and the new podcastTitle field. The shared-module change is genuinely additive (podcast_title is already non-null in CombinedSearchResponse, and ImprovedSearchResultItem.EpisodeItem has no other construction sites outside ImprovedSearchManagerImpl).

A handful of things worth addressing before merge, then some nits.


1. tv_search_searching is unused — dead string going to translators

modules/services/localization/src/main/res/values/strings.xml:256 adds Searching…, but nothing references it (the Searching state renders LoadingView). Since strings ship to GlotPress, drop it or use it.

Fix this →

2. TvSearchEpisodeRow's wrapper is a verbatim copy of TvEpisodeListItem

tv/.../search/TvSearchEpisodeRow.kt:51-87 (the isItemFocused row + AnimatedVisibility more-button, including the private MORE_BUTTON_ANIMATION_DURATION_MS = 200 constant and the full enter/exit spec) is character-for-character tv/.../component/TvEpisodeListItem.kt:30-73. Only the inner row differs (TvSearchEpisodeCard vs TvEpisodeRow).

Worth extracting the wrapper into a slot-based component in component/ — e.g. TvEpisodeListItem(onOpenActions, …, content: @Composable (Modifier) -> Unit) — and having both call sites use it. That also keeps the reveal animation from drifting between screens.

Fix this →

3. Error state shows no-results copy, and there's no retry

TvSearchScreen.kt:179-182 renders the network-failure case as "Something went wrong" + "Try more general or different keywords." — the subtitle is misleading for a dropped connection, and there's no way to retry short of editing the query. TvEmptyState already supports actionLabel/onAction; a dedicated subtitle plus a Retry that re-runs onQueryChange(query) would be a small addition.

Fix this →

4. Focus restore after the overlay closes only works when there are podcast results

TvSearchScreen.kt:252-268: podcastsRowFocusRequester is only attached when results.podcasts.isNotEmpty(), but restoreFocusTrigger always tries to focus it. For an episodes-only result set the requestFocus() throws and is swallowed by runCatching, so after closing a podcast overlay nothing regains focus — and that path is reachable: episode "…" → Go to podcast → back.

Even with podcasts present, coming back from an episode's Go to podcast dumps focus on the podcasts carousel rather than the episode row the user was on. Tracking which row opened the overlay (or falling back to the first episode row when podcasts is empty) would fix both.

Fix this →

5. TvSearchSectionTitle re-implements TvRow's title instead of sharing it

TvSearchScreen.kt:315-327 copies TvRow's exact title style (17.sp / FontWeight(500) / includeFontPadding = false, bottom = 17.dp) and hardcodes start = 48.dp instead of using the file's ContentPadding. Two consequences: the padding silently drifts if ContentPadding changes, and the Episodes header won't animate to 21.sp on focus the way the Podcasts header does (TvRow.kt:40-41,56-58), so the two headers will look inconsistent as focus moves down. Extracting the title out of TvRow into a shared TvSectionTitle(title, contentPadding, isFocused) would cover both.


Nits / questions

  • Episodes aren't de-duplicated by uuid (TvSearchScreen.kt:275-277 keys on EpisodeItem::uuid). Podcasts get an explicit distinctBy in the view model; episodes don't. If the combined endpoint ever returns the same episode twice, items(key = …) throws on duplicate keys. A .distinctBy { it.uuid } in TvSearchViewModel.kt:110 is cheap insurance.
  • Visual divergence from TvEpisodeRow: the search card uses 96.dp artwork / 24.dp padding / body + caption1 / backgroundSunken / 12.dp corners, versus 82.dp / 12.dp / callout + caption2 / backgroundBase / 8.dp in TvEpisodeRow. Deliberate (iOS parity) or drift?
  • isFollowed and publishedDate are carried but never rendered. Podcast.toSearchItem() sets isFollowed = true and the tile has no followed indicator, so the flag only affects dedupe ordering; EpisodeItem.publishedDate isn't shown in the row. Fine if that matches the iOS design — just noting the unused data.
  • Test gaps. The state-machine tests are good, but the fiddliest logic is untested: hydrate()'s fallback (findByUuidfindOrDownloadPodcastRxSinglefindByUuid) and the playStarted / playFailures emissions in playEpisode / openEpisodeActions. Also, with the debounce stubbed to 0, nothing exercises cancel-on-keystroke — a test that fires two onQueryChange calls inside the debounce window and asserts only the last term is searched would lock in the intended behaviour.
  • Duplication with TvHomeViewModel — agreed with your follow-up note; it's now playEpisode + playStarted/playFailures + the hydrate helper in two places, and TvSearchViewModel is up to seven constructor dependencies. Worth extracting before a third tab copies it.
  • Analytics: no tracking on search performed / result selected, but there's no AnalyticsTracker usage anywhere in tv/, so this is consistent with the rest of the module. SourceView.SEARCH_RESULTS for playback + the new TvEpisodeActionContext.SearchResults look right.
  • TvSearchField.kt:73-79 returning focusManager.moveFocus(...) directly is the right call — when nothing is focusable below (Searching / Error / NoResults) it returns false and the event falls through instead of trapping the user in the field. 👍

I didn't run Gradle (spotlessCheck / :tv:testDebugUnitTest) in this environment, so the tests are reviewed by reading only.
• branch feat/tv-search-results

@geekygecko geekygecko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sztomek
sztomek force-pushed the feat/tv-home-auth-feeds branch from 425f5e8 to a1420f6 Compare August 14, 2026 07:28
Base automatically changed from feat/tv-home-auth-feeds to main August 14, 2026 07:38
@sztomek
sztomek force-pushed the feat/tv-search-results branch from 7bc70d4 to 17d971b Compare August 14, 2026 08:01
@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

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


Re-review after 17d971b (Address PR comments)

  • Diff the new commits against my previous review
  • Verify each earlier finding is addressed
  • Look for new issues in the updated code
  • Post the review

Everything I raised last time is addressed, and the fixes are the right shape rather than the minimum patch:

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 findOrDownloadPodcastRxSingle calls. Same for playEpisode at :126 — repeated Enter on a row queues multiple playNowSuspend calls.

A single MutableStateFlow<String?> of the in-flight episode uuid would drive a spinner and let both functions early-return while one is pending.

Fix this →

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 the LazyColumn's composed window, requestFocus() throws and runCatching swallows 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 a Modifier down through a slot works, but content: @Composable RowScope.() -> Unit is the more idiomatic shape: callers write Modifier.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-116 calls focusManager.moveFocus(FocusDirection.Down) and returns true regardless of the result, so in Searching and NoResults — 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 the moveFocus result 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 the playStarted / playFailures emissions 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 / publishedDate are 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

@sztomek
sztomek merged commit 89cb3c6 into main Aug 14, 2026
19 checks passed
@sztomek
sztomek deleted the feat/tv-search-results branch August 14, 2026 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants