Skip to content

[TV] Add category details - #5731

Merged
sztomek merged 2 commits into
mainfrom
feat/tv-search-category-details
Aug 14, 2026
Merged

[TV] Add category details#5731
sztomek merged 2 commits into
mainfrom
feat/tv-search-category-details

Conversation

@sztomek

@sztomek sztomek commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Description

Stacked on #5730 (feat/tv-home-discover-parity). Adds the Search-screen half of the TV Discover category-details work: the shared category-detail screen and feed loader land in the base PR; this wires them into Search and polishes the browse UX.

  • Category pills → detail. The Search "Browse categories" pills were a no-op (onClick = {}); they now open the shared TvCategoryPodcastsScreen via the same TvDetailOverlay stacking used on Home. TvSearchViewModel.categoryPodcasts loads the grid (region resolution + sponsored merge come from the base loader).
  • Category overlay fills the screen. TvScaffold no longer wraps the Search tab in belowTopBar padding; Search pads its own content instead, so the category/podcast detail overlays fill the full height. Previously a category opened from Search left a large empty gap under the (hidden) top bar.
  • Search field scrolls away. The search field moves into the idle browse LazyColumn (first item) so it scrolls with the categories instead of staying pinned. TvSearchField autofocus is made one-shot (survives via a stable list-item key) so scrolling back to the top doesn't re-open the keyboard.
  • Focus restore on return. Returning from a category (or podcast) overlay to the idle browse screen restores focus to the last-focused row (mirroring TvHomeScreen), instead of the top-bar profile icon stealing focus.

Fixes POC-840 https://linear.app/a8c/issue/POC-840/category-details

Testing Instructions

  1. Open the Search tab; the field auto-focuses (keyboard up). Press Down to reach the Browse categories pills.
  2. Scroll down — the search field scrolls away with the content; scroll back up — the keyboard does not re-pop.
  3. Press a category pill → "Most Popular in {category}" grid opens with no empty gap at the top; open a podcast; Back twice.
  4. On Back to Search, focus lands on the categories row, not the profile icon.

Screenshots or Screencast

Screen_recording_20260812_204800.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 changed the title [TV] Integrate category details into the Search screen [TV] Add category details Aug 12, 2026
@sztomek sztomek added this to the 8.19 milestone Aug 12, 2026
@sztomek sztomek added [Type] Feature Adding a new feature. [Area] TV labels Aug 12, 2026
@sztomek
sztomek marked this pull request as ready for review August 12, 2026 19:02
@sztomek
sztomek requested a review from a team as a code owner August 12, 2026 19:02
@sztomek
sztomek requested review from geekygecko and removed request for a team August 12, 2026 19:02
@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 48s —— View job


Review: [TV] Add category details

  • Read the PR diff against feat/tv-home-discover-parity
  • Read the touched files and their collaborators (TvHomeScreen, TvDetailOverlay, TvCategoryPodcastsScreen, TvRow, tvDiscoverRow)
  • Analyze correctness / focus-management / state issues
  • Post review

The wiring mirrors TvHomeScreen closely, which is the right call — the overlay stacking, the two restore triggers, and the tvFocusInactiveWhen layering all match the Home implementation exactly, so the Back/focus behaviour should be consistent between the two tabs. The TvScaffold change is correct too: belowTopBar now only wraps UpNext, TvSearchScreen pads its own content with TvTopBarHeight, and the comment above belowTopBar still describes the rule accurately. Nothing here blocks merge; below is what I'd look at.


1. The search field is now two different BasicTextField instances

TvSearchScreen.kt:195-204 (non-idle) and TvSearchScreen.kt:282-292 (idle) each call TvSearchField. Before this PR the field lived above the if, so a single instance persisted across every state. Now the instance is destroyed and recreated on every Idle ↔ non-Idle transition, which happens while the user is typing (onQueryChange → debounce → Searching, TvSearchViewModel.kt:109-111).

Consequences worth checking on a real device with the leanback IME:

  • The BasicTextField is recreated, so its IME connection restarts and its internal selection/composing state is dropped mid-word (the text itself is safe — it's hoisted in the VM).
  • There is a frame where the focused node has been disposed and the replacement hasn't requested focus yet; on TV that can flash focus onto the top bar.
  • The new instance has autoFocus = true with a fresh hasAutoFocused = false, so keyboardController.show() fires again on the first Idle → Searching transition.

The most robust shape is one field instance that both layouts share — e.g. keep the field in TvSearchContent for the non-idle branch only if you also stop rendering it in the lazy list, or hoist a TextFieldValue into the VM so the swap is lossless. Worth at least a manual pass of "type a query slowly, keep typing across the debounce boundary".

2. hasAutoFocused latches even when requestFocus() failed

TvSearchField.kt:59-65:

runCatching { focusRequester.requestFocus() }
keyboardController?.show()
hasAutoFocused = true

requestFocus() on a node that isn't attached/placed yet throws, and runCatching swallows it — but the flag is set anyway. Now that the field is a lazy-list item (composed and measured as part of LazyColumn's first pass, rather than a plain Column child), that first request is more timing-sensitive than it was. If it loses the race once, the field never auto-focuses and the keyboard never comes up for the lifetime of that list item — the failure is silent and permanent. Gating on success costs one line:

if (runCatching { focusRequester.requestFocus() }.isSuccess) {
    keyboardController?.show()
    hasAutoFocused = true
}

Fix this →

Also note hasAutoFocused is rememberSaveable, so it survives activity recreation — after a config change / process restore the field deliberately won't re-focus. That's probably what you want, just flagging it's a behaviour difference from remember (which wouldn't survive the lazy-item disposal you need it for).

3. onPlayEpisode = {} is still a no-op in the Search discover feed

TvSearchScreen.kt:324. This PR wired onOpenPodcast and onOpenCategory on the same call, which makes the remaining stub more visible: a TvDiscoverRow.Episodes row (e.g. "Made for TV") renders TvVideoTiles whose Go to podcast action now works but whose Play action silently does nothing. TvHomeScreen passes viewModel::playEpisode here. The blocker is the type mismatch — TvSearchViewModel.playEpisode takes ImprovedSearchResultItem.EpisodeItem, not TvDiscoverEpisode — so it needs a small VM overload. Either add it or leave a TODO so it isn't mistaken for intentional.

4. TvSearchScreen is now a near-verbatim copy of TvHomeScreen

Two blocks are duplicated exactly:

  • Overlay stacking: TvSearchScreen.kt:129-153 vs TvHomeScreen.kt:101-125 — same two overlays, same tvFocusInactiveWhen(podcastUuid != null), same onHide = { if (openedCategory != null) categoryRestoreTrigger++ else restoreFocusTrigger++ }. That last rule is subtle enough that having it in two places is a real drift risk; a shared TvCategoryAndPodcastOverlays(...) in component/discover taking the two states plus getCategoryPodcasts would collapse ~25 lines and keep the tabs in sync.
  • Row focus restore: TvSearchScreen.kt:267-279 vs TvHomeScreen.kt:194-205 — identical rowFocusRequesters / lastFocusedRowIndex / isInitialComposition / LaunchedEffect(restoreFocusTrigger) quartet. This is now the 4th variant of the pattern in the TV module (TvSearchTopResults:403-411, TvSearchEpisodeGrid:455-462 are two more). A rememberTvRowFocusRestore(rowCount, restoreFocusTrigger) helper returning the requesters plus a per-index modifier would cover all of them.

One behavioural note on the shared pattern: runCatching { ... requestFocus() } means that if the last-focused row was disposed by the LazyColumn while the overlay was up, focus is silently lost and lands back on the top bar — the exact symptom this PR set out to fix. In practice the list keeps its composition behind the overlay so it should hold, but it's worth a test with a long feed and a row scrolled well off screen.

5. Smaller things

  • TvSearchContent's onOpenCategory: (DiscoverCategory) -> Unit = {} (TvSearchScreen.kt:191) defaults to a silent no-op — the same footgun that produced the onClick = {} bug this PR is fixing. Only the preview relies on the default; making it required (and moving it above modifier) makes a future miss a compile error. Same for TvSearchDiscover's restoreFocusTrigger: Int = 0 (:265), which has exactly one caller that always passes it.
  • The search-field block (Spacer(40.dp) / field / Spacer(24.dp) inside Column(padding(ContentPadding))) is byte-identical at :196-204 and :283-291 — extract a private TvSearchFieldHeader so the two states can't drift in spacing.
  • TvSearchViewModel.categoryPodcasts (:136-138) has no test, while the identical TvHomeViewModel.categoryPodcasts has three (TvHomeViewModelTest.kt:419-457) and TvSearchViewModelTest already builds a real TvDiscoverFeedLoader (:274). It's a thin delegate so the loader logic is covered either way, but one mapping test is nearly free.
  • The TvSearchContent preview doesn't apply the padding(top = TvTopBarHeight) the real caller now supplies (:124-127), so the preview's vertical layout no longer matches the device. Moving that padding inside TvSearchContent would keep the preview honest — though it'd diverge from how TvHomeScreen does it, so it's a toss-up.

I didn't build or run tests (no Gradle run in this environment), so all of the above is from reading the code.
• branch feat/tv-search-category-details

@sztomek
sztomek force-pushed the feat/tv-search-category-details branch from a18b737 to 44cb8bc Compare August 12, 2026 19:11

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

I mentioned this on an earlier PR. I think the user should have to tap the search field before the keyboard pops up.

@sztomek
sztomek force-pushed the feat/tv-home-discover-parity branch 2 times, most recently from ce416b5 to f4ee971 Compare August 14, 2026 10:34
Base automatically changed from feat/tv-home-discover-parity to main August 14, 2026 10:45
@sztomek
sztomek force-pushed the feat/tv-search-category-details branch from 44cb8bc to e9ef950 Compare August 14, 2026 11:02
@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

Claude finished @sztomek's task in 5m 44s —— View job


Re-review: [TV] Add category details

  • Diff the branch against the merged base (#5730 is now in main)
  • Review what changed since my last pass
  • Re-check the open points from the previous review
  • Post review

The branch has been rebased onto main since my last pass, so the diff is materially different from what I reviewed on Aug 12. Most of what I raised then is resolved: onOpenCategory is now a required parameter on TvSearchContent, the onPlayEpisode stub carries a TODO, categoryPodcasts has a test, and TvSearchField is now the click-to-edit tile — the keyboard no longer pops on entry, which is what @geekygecko asked for. The overlay stacking, the two restore triggers and the tvFocusInactiveWhen layering match TvHomeScreen exactly, so Back/focus behaviour should be consistent across the two tabs.

Nothing here blocks merge. Two inline comments on the code; the rest is below.


1. The PR description no longer matches the code

Two bullets describe work that isn't in the current diff:

Search field scrolls away. The search field moves into the idle browse LazyColumn (first item)…
TvSearchField autofocus is made one-shot (survives via a stable list-item key)…

TvSearchScreen.kt:197-206 keeps the field in a fixed Column above the scrollable area — a single instance, outside the LazyColumn — and TvSearchField no longer has an autoFocus parameter at all. TvSearchContent.kt:192-196 focuses the tile (restFocusRequester), so the field is focused but not in editing mode and the IME stays down until the user presses Select. That's better than what the description promises, but testing steps 1 and 2 now describe behaviour that doesn't exist ("the field auto-focuses (keyboard up)", "scroll back up — the keyboard does not re-pop").

Worth fixing before merge since the description becomes the squash commit message.

2. onOpenCategory on tvDiscoverRow is unreachable in Search

TvSearchScreen.kt:317. Search builds its rows with buildRows(discover, isLoggedIn), i.e. includeHomeSections = false, and TvDiscoverFeedLoader.kt:112 returns null for ListType.Categories in that mode. So TvDiscoverRow.Categories can never appear in discoverRows and this callback can never fire — the pills row at :296-305 is the only live path to a category.

Harmless and arguably correct as future-proofing, just worth knowing when testing: there is exactly one entry point to exercise.

3. Sponsored ads for a Search category come from the home feed

TvSearchViewModel.kt:137-139loadCategoryPodcastsloadCategorySponsoredPodcastslistRepository.getHomeDiscoverFeed(isLoggedIn) (TvDiscoverFeedLoader.kt:62). Search's rows and categories come from getSearchDiscoverFeed(), so a category opened from Search resolves its region and its sponsored rows against a different feed than the one that produced the category. The new test makes this visible — it has to stub getHomeDiscoverFeed to exercise a Search code path (TvSearchViewModelTest.kt:211).

This is the base loader's design and matches Home, so I assume it's intentional; a one-line comment on categoryPodcasts explaining why the home feed is the sponsored source would save the next reader the trip through the loader.

4. Smaller things

  • TvSearchScreenPreview (:545+) still calls TvSearchContent without the padding(top = TvTopBarHeight) the real caller now supplies (:124), so the preview's vertical layout no longer matches the device. Moving the padding inside TvSearchContent would fix it, at the cost of diverging from how TvHomeScreen does it.
  • TvSearchDiscover's restoreFocusTrigger: Int is required now (good), but TvSearchContent's is still = 0 (:190) with a single caller that always passes it.
  • The TvScaffold change is correct: belowTopBar now only wraps UpNext, the comment above it (:88-89) still describes the rule accurately, and the two modals in TvSearchScreen are Dialog-based (TvModal.kt:45) so losing the wrapper Box doesn't shift them.

5. Worth one manual check

TvSearchContent.kt:192-196 requests focus on the search-field tile from LaunchedEffect(Unit), and runCatching swallows a failure with no retry. openedCategory is rememberSaveable, so after process death with a category open, TvSearchContent composes fresh and fires that request while its subtree is canFocus = false (:125). It should lose cleanly to the grid's autoFocusFirstItem, but it's the one ordering I can't reason about with confidence from reading — worth killing the process with a category grid open and checking where focus lands on restore.


I could not build or run tests in this environment (Gradle has no dependency cache or network access here), so everything above is from reading the code. Worth running ./gradlew :tv:testDebugUnitTest and ./gradlew spotlessCheck locally if CI hasn't already.
• branch feat/tv-search-category-details

@sztomek
sztomek merged commit a394ecc into main Aug 14, 2026
23 checks passed
@sztomek
sztomek deleted the feat/tv-search-category-details branch August 14, 2026 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Area] TV [Type] Feature Adding a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants