Skip to content

[TV] Search: Podcasts/Episodes scope filter - #5729

Open
sztomek wants to merge 3 commits into
feat/tv-search-resultsfrom
feat/tv-search-filter
Open

[TV] Search: Podcasts/Episodes scope filter#5729
sztomek wants to merge 3 commits into
feat/tv-search-resultsfrom
feat/tv-search-filter

Conversation

@sztomek

@sztomek sztomek commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Description

Adds the Podcasts/Episodes scope filter to the TV Search screen, on top of the basic search PR.

Stacked on #5728 (feat/tv-search-results). Review/merge that one first.

  • Pill filterTop Results / Podcasts / Episodes, styled exactly as the top‑bar tab pills (androidx.tv.material3 TabRow + pill indicator in a backgroundSunken container), centered between two divider lines per the Figma. Selecting follows focus, like the top bar.
  • Scopes:
    • Top Results — the combined view (podcasts carousel + a capped episodes preview).
    • Podcasts — a 6‑column cover grid (TvPodcastGridScaffold).
    • Episodes — the full episode list.
  • Adds filter state + onFilterSelected to TvSearchViewModel and the scope switching in TvSearchScreen. No new strings (reuses search_filters_*), no analytics.

Fixes POC-800 https://linear.app/a8c/issue/POC-800/wire-up-search-apis
Figma: Ftk3KwnfqaK4g57yCN63p0-fi-2595_2243

Testing Instructions

  1. Build the branch (stacked on [TV] Search: combined search results (podcasts + episodes) #5728) and open the Search tab; type a query.
  2. A centered Top Results / Podcasts / Episodes pill bar appears between two divider lines.
  3. D‑pad down from the field lands on the filter; arrow left/right to switch scope (selects on focus).
  4. Top Results shows podcasts + an episodes preview; Podcasts shows the cover grid; Episodes shows the full list.
  5. Clearing the query hides the filter and returns to the Discover browse.

Screenshots or Screencast

Screen_recording_20260812_142126.mp4

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.

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@sztomek
sztomek force-pushed the feat/tv-search-filter branch from af82e68 to 3414c70 Compare August 12, 2026 11:49
@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:05
@sztomek
sztomek requested a review from a team as a code owner August 12, 2026 12:05
@sztomek
sztomek requested review from geekygecko and removed request for a team August 12, 2026 12:05
@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 2m 33s —— View job


Code review: TV Search Podcasts/Episodes scope filter

  • Gather context (diff vs feat/tv-search-results)
  • Review TvSearchFilters.kt, TvSearchScreen.kt, TvSearchViewModel.kt, TvPodcastGridScaffold.kt
  • Post review

Nice, focused change — the scope switching reads cleanly and splitting TvSearchResults into TvSearchTopResults / TvSearchEpisodeList was the right call. A few things worth looking at before merge, in rough priority order.


1. Focus is lost in the Episodes scope after the podcast overlay closes

restoreFocusTrigger exists precisely because tvFocusInactiveWhen(podcastUuid != null) (TvSearchScreen.kt:108) strips focus from the content while the detail overlay is up, and onHide (:113) bumps the trigger so something below re-claims it.

TvSearchTopResults consumes it (:329) and TvPodcastGridScaffold consumes it (:290), but TvSearchEpisodeList (:368-385) doesn't take it at all. Repro on the Episodes tab: focus an episode → open actions → Go to podcast → back out of the podcast detail. The overlay closes, nothing requests focus, and the D-pad goes dead until the user finds another way in.

Related, milder: in TvSearchTopResults the restore always targets podcastsRowFocusRequester, which is only attached when podcasts.isNotEmpty() (:338). Episode-only results hit the same dead-end — runCatching (:333) swallows it silently. Worth a fallback to the first episode row.

Fix this →

2. No explicit focus requester on the selected pill

TvTabBar pins focus to the selected tab (TvTabBar.kt:107, .then(if (index == selectedTabIndex) Modifier.focusRequester(focusRequester) else Modifier)). TvSearchFilterPills relies on focusRestorer() alone.

Since selection follows focus here, any case where the restorer's saved child is dropped means focus lands on Top Results and the whole content region silently swaps out from under the user. Worth exercising: select Episodes → D-pad down into the list → scroll a few items → D-pad up back to the pills. If focus doesn't return to Episodes, the scope resets.

3. title = "" as a "no title" sentinel

TvSearchScreen.kt:287 passes an empty string, and TvPodcastGridScaffold.kt:41 now branches on title.isNotEmpty(). An empty-string sentinel is easy to trip over — title: String? = null expresses it directly and lets the search call site just omit the argument. (The other two call sites, TvFolderDetailScreen.kt:98 and TvYourPodcastsScreen.kt:162, pass real titles and are unaffected.)

Fix this →

4. Podcasts grid is misaligned with the rest of the search screen

The search field, filter row and Top Results carousel all sit at 48dp (ContentPadding, TvSearchScreen.kt:63), but TvPodcastGridScaffold hardcodes contentPadding = PaddingValues(start = 32.dp, …, end = 32.dp) (TvPodcastGridScaffold.kt:77). Switching Top Results → Podcasts shifts the covers 16dp left relative to the pills above them. Either plumb a contentPadding parameter through the scaffold or wrap the grid — worth a quick check against the Figma.

5. Duplicated pill-bar implementation

TvSearchFilterPills (TvSearchFilters.kt:52-108) is a near-verbatim copy of TvTabBar.kt:79-157 — same Box + backgroundSunken + RoundedCornerShape(percent = 50) + 3.dp padding, same TabRow/PillIndicator config, same TabDefaults.pillIndicatorTabColors five-colour block, same 44.dp/21.dp metrics, same inner Box + caption1 Text. That's the styling contract for TV pills duplicated in two files, so any design tweak now has to land in both.

Extracting a shared TvPillTabRow(labels, selectedIndex, onSelect, …) into component/ would let both call sites keep only their own focus behaviour. Not blocking, but the divergence risk is real given how mechanically identical these are.

6. Filter shown for Searching / Error / NoResults

searchState !is TvSearchState.Idle (TvSearchScreen.kt:170) also covers the loading spinner, the error message and the no-results message. In all three the pills are interactive but change nothing, and on Error/NoResults they're a focusable row leading to identical content. Intentional (avoids the row flickering on every keystroke while Searching), or should it be is TvSearchState.Results?

7. Smaller notes

  • Modifier.padding(horizontal = 48.dp) at TvSearchScreen.kt:174 duplicates the ContentPadding constant declared 111 lines above — use Modifier.padding(ContentPadding) so they can't drift.
  • The no-results message is written three times (:281-284, :304-307) with identical arguments. A local @Composable fun NoResults() or hoisting it above the when would cut the repetition.
  • Episode item rendering is duplicated between TvSearchTopResults (:350-361) and TvSearchEpisodeList (:374-382) — a LazyListScope.tvSearchEpisodeItems(episodes, …) extension, mirroring the existing tvSearchPodcastsRow, would cover both.
  • color = LocalContentColor.current on the Text (TvSearchFilters.kt:97) is the default already — harmless, and it matches TvTabBar, so only worth dropping if you extract the shared component.
  • Preview coverage: only TvSearchFilter.Podcasts is previewed. A TopResults variant would catch first-pill indicator/rounding issues.

8. Tests

TvSearchViewModelTest is already well built out, and the two new VM behaviours are trivially testable with the existing harness — no new mocks needed:

  • onFilterSelected updates filter.
  • Clearing the query resets filter to TopResults (TvSearchViewModel.kt:104), which is the non-obvious one and easy to regress.

Fix this →


Verdict: the only thing I'd call blocking is #1 — a dead D-pad on TV is a hard stop for the user. #2 is worth a manual pass on device since it's cheap to verify. Everything else is polish and can land as follow-ups.

Note: I reviewed the diff statically and did not build or run the module.
• branch feat/tv-search-filter

@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.

Works well. The only minor change is that it needs to use the grey state similar to Apple TV when you are on the control.

Not hovering:

Image

Hovering:

Image

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.

2 participants