Skip to content

[TV] Add the Your Podcasts tab - #5680

Open
sztomek wants to merge 7 commits into
feat/tv-up-nextfrom
feat/tv-your-podcasts
Open

[TV] Add the Your Podcasts tab#5680
sztomek wants to merge 7 commits into
feat/tv-up-nextfrom
feat/tv-your-podcasts

Conversation

@sztomek

@sztomek sztomek commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Description

Adds the Your Podcasts tab to the TV app, mirroring the Apple TV PodcastsView. This PR covers the two states in scope — the empty view and the populated podcasts grid. Folders are handled as a follow-up (stacked on this branch).

  • Grid: subscribed podcasts are shown as a 6-column grid of square artwork tiles, matching tvOS's LazyVGrid of 6 fixed columns. Focus scales the tile and the last-focused position is restored when re-entering the grid (the same focus pattern as the Playlists tab).
  • Data: observes PodcastManager.findSubscribedFlow() and sorts with PodcastsSortType.NAME_A_TO_Z — the article-aware A→Z order tvOS uses (HomeGridDataHelper.gridItems(orderedBy: .titleAtoZ)). The list is kept fresh reactively from the DB (synced at sign-in); see the note below on why the tab doesn't force its own network refresh.
  • Empty state: "Time to fill this up" with a Discover podcasts button that jumps to the Home tab, matching tvOS's ContentUnavailableView action (tabRouter.selectedTab = .home).
  • Reuse: generalized the existing TvPodcastTile with an imageModifier (defaulting to the current fixed 123.dp used by the Home rows) so the same component fills a grid cell here — this also gives each tile a proper artwork content description.

Scope / deliberate deviations (consistent with the rest of the TV module):

  • The screen title is pinned above the grid rather than scrolling with it as on tvOS — this matches the sibling Playlists / Up Next tabs, favouring platform-internal consistency.
  • Tapping a podcast is a no-op — the podcast detail screen isn't wired anywhere on TV yet, and it's out of scope for this ticket (grid + empty view only). The tiles remain focusable, matching the Home tab tiles.
  • No analytics — the TV module has none; adding the tvOS podcastsListShown events would need an EventHorizon schema change and would be inconsistent with the sibling tabs.

Fixes PCDROID-696 https://linear.app/a8c/issue/PCDROID-696/podcasts-grid-and-empty-view.
Designs: Ftk3KwnfqaK4g57yCN63p0-fi-2052_15345.

Stacked on feat/tv-up-next (#5679) — merge bottom-up.

Testing Instructions

  1. Install the TV app (./gradlew :tv:installDebugProd) and sign in with an account that follows several podcasts.
  2. Open the Your Podcasts tab — verify the followed podcasts appear as a 6-column grid, sorted A→Z (ignoring a leading "The"), under a "Your Podcasts" title.
  3. Navigate the grid with the d-pad — the focused tile scales up; leave and re-enter the grid and verify focus returns to the last-focused tile.
  4. Empty state: with an account that follows no podcasts, verify "Time to fill this up" with a focused Discover podcasts button; selecting it switches to the Home tab.
  5. Navigate in and out of the tab repeatedly — verify it never crashes (regression check for the token-refresh crash described above).

Screenshots or Screencast

Screen_recording_20260729_185640.mp4

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md (TV app is unreleased — skipped)
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes (added TvYourPodcastsViewModelTest)
  • 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. (analytics deliberately excluded — matches the rest of the TV module)

@sztomek sztomek added [Type] Feature Adding a new feature. [Area] TV labels Jul 29, 2026
@sztomek sztomek added this to the 8.18 milestone Jul 29, 2026
@sztomek
sztomek marked this pull request as ready for review July 29, 2026 16:58
@sztomek
sztomek requested a review from a team as a code owner July 29, 2026 16:58
@sztomek
sztomek requested review from geekygecko and removed request for a team July 29, 2026 16:58
@sztomek

sztomek commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

@claude please review this

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


Code Review

  • Gather context (diff vs origin/feat/tv-up-next)
  • Review new screen + ViewModel + test
  • Check shared component / scaffold changes
  • Post review

Nice, tight PR — it slots cleanly into the patterns the Playlists/Up Next tabs established, the deliberate deviations are all called out in the description, and the TvPodcastTile generalisation is the right call over a second near-duplicate tile. Two things worth fixing before merge, and a handful of nits.


🟠 1. Sorting runs on the main thread on every DB emission

tv/src/main/java/au/com/shiftyjelly/pocketcasts/podcasts/TvYourPodcastsViewModel.kt:22-32

val uiState: StateFlow<TvYourPodcastsUiState> = podcastManager.findSubscribedFlow()
    .map { podcasts ->
        val sorted = podcasts.sortedWith(PodcastsSortType.NAME_A_TO_Z.podcastComparator)
        ...
    }
    .stateIn(viewModelScope, ...)

Room's flow has its own flowOn(queryDispatcher) upstream, so this map executes in the collector's context — viewModelScope, i.e. Dispatchers.Main.immediate. The work isn't trivial:

// PodcastsSortType.kt:16-18, :58
podcastComparator = compareBy { cleanStringForSortInternal(it.title) }
private fun cleanStringForSortInternal(value: String) =
    value.lowercase(Locale.getDefault()).replaceFirst(replaceTheRegex, "").removeAccents()

compareBy invokes the selector on every comparison, not once per element — so it's O(n log n) lowercase + regex + removeAccents() calls, on the main thread, re-run on every findSubscribedFlow() emission. Room re-emits on any change to the podcasts table, which during a sync is many times in a row. On TV hardware with a few hundred subscriptions that's a visible jank source.

Worth noting every other podcastComparator use in the repo is inside a suspend function already off the main thread (PodcastManagerImpl.kt:351, :415) — this is the first main-thread one.

Cheapest fix is .flowOn(Dispatchers.Default) before stateIn; you can additionally decorate-sort-undecorate to compute each key once:

.map { podcasts ->
    val sorted = podcasts
        .map { it to PodcastsSortType.cleanStringForSort(it.title) }
        .sortedBy { (_, key) -> key }
        .map { (podcast, _) -> podcast }
    if (sorted.isEmpty()) TvYourPodcastsUiState.Empty else TvYourPodcastsUiState.Loaded(sorted)
}
.flowOn(Dispatchers.Default)
.stateIn(...)

Fix this →


🟠 2. onEnter can crash on an uninitialized FocusRequester

tv/src/main/java/au/com/shiftyjelly/pocketcasts/podcasts/TvYourPodcastsScreen.kt:109-121

var lastFocusedIndex by rememberSaveable(podcasts.size) { mutableIntStateOf(0) }
val focusRequesters = remember(podcasts.size) { List(podcasts.size) { FocusRequester() } }
...
onEnter = { focusRequesters.getOrNull(lastFocusedIndex)?.requestFocus() }

getOrNull guards the index range, but not the thing that actually throws: FocusRequester.requestFocus() raises IllegalStateException("FocusRequester is not initialized…") when no node is attached to it. A LazyVerticalGrid only composes visible items, so requesters for off-screen tiles are never attached.

Concrete repro:

  1. User with ~100 podcasts scrolls to the bottom of the grid, focuses a tile, then d-pads up to the tab bar. lastFocusedIndex ≈ 95, scroll position is near the end.
  2. A sync unsubscribes (or adds) one podcast → podcasts.size changes → rememberSaveable(podcasts.size) re-initialises lastFocusedIndex to 0. The internal rememberLazyGridState() is not keyed on the list, so the grid stays scrolled to the bottom.
  3. User presses down → onEnterfocusRequesters[0].requestFocus() → item 0 isn't composed → crash.

This code is inherited from TvPlaylistsScreen.kt:190-201, where it's effectively unreachable: a 3-column grid of a handful of playlists never scrolls. A 6-column podcasts grid scrolls routinely, so the latent bug becomes live here.

Tracking the uuid instead of the index fixes both this and the reorder mismatch in note 4 below:

val gridState = rememberLazyGridState()
var lastFocusedUuid by rememberSaveable { mutableStateOf<String?>(null) }
val focusRequesters = remember(podcasts.size) { List(podcasts.size) { FocusRequester() } }
...
onEnter = {
    val visible = gridState.layoutInfo.visibleItemsInfo
    val target = podcasts.indexOfFirst { it.uuid == lastFocusedUuid }
        .takeIf { index -> visible.any { it.index == index } }
        ?: visible.firstOrNull()?.index
    target?.let { focusRequesters.getOrNull(it)?.requestFocus() }
}

(and pass state = gridState to the LazyVerticalGrid, set lastFocusedUuid = podcast.uuid in onFocusChanged).

Fix this →


Nits & suggestions

3. Three copies of the same empty state. TvYourPodcastsEmpty (TvYourPodcastsScreen.kt:163-201) is character-for-character UpNextEmpty (TvUpNextScreen.kt:182-220) with different strings, and TvPlaylistsEmpty is the same shape again. Good moment to pull a TvEmptyState(title, subtitle, actionLabel, onAction) into component/ — the third copy is usually the signal.

4. rememberSaveable(podcasts.size) vs Playlists' rememberSaveable(playlists). Keying on .size is a genuine improvement over the sibling — Podcast is a data class with many fields that churn during sync (syncStatus, latestEpisodeUuid, …), so keying on the whole list would blow away focus memory on unrelated DB writes. The remaining gap: if the list changes content but not size (a title edit that re-sorts, or an unsubscribe+subscribe in one emission), the stored index points at a different podcast. The uuid-based approach in item 2 covers this.

5. Empty state steals focus, grid doesn't. TvYourPodcastsEmpty does LaunchedEffect(Unit) { focusRequester.requestFocus() }, so simply d-padding sideways along the tab bar onto this tab yanks focus down into the Discover podcasts button. The grid path deliberately doesn't do that (it only reacts to onEnter). It's copied verbatim from UpNextEmpty so it's consistent with the sibling — flagging in case the divergence within one screen isn't intentional.

6. String naming. tv_podcasts_empty_* doesn't match the screen — the siblings mirror theirs (tv_playlists_empty_*, tv_up_next_empty_*). tv_your_podcasts_empty_* would be consistent, and renaming is free now but costly once GlotPress has picked them up.

7. imageModifier: Modifier = Modifier.width(123.dp) (TvPodcastTile.kt:25). A Modifier parameter whose default isn't plain Modifier is a bit of an API smell — a caller passing Modifier.padding(4.dp) silently loses the 123.dp width with no hint. With only three call sites, either imageModifier: Modifier = Modifier with .width(123.dp) moved to TvHomeScreen.kt:145 / TvTabPlaceholder.kt:78,91, or a size: Dp = 123.dp param, would be clearer. Not blocking.

8. Grid title reuses the tab-bar stringstringResource(LR.string.tv_tab_your_podcasts) at TvYourPodcastsScreen.kt:105. Works, but couples the tab label to the screen heading; Playlists uses the generic LR.string.playlists. Fine either way.

9. private const val GRID_COLUMNS = 6 sits between the composables and the previews (:203) — conventionally top-of-file or after the previews.

10. Tests. Both tests are well chosen, and the "The Beat" case pinning article-aware ordering is exactly the right thing to assert. Two gaps: no Loaded → Empty transition (unsubscribing the last podcast), and no Loading-state preview to go with the grid/empty ones.


What's good

  • Reusing TvPodcastTile rather than forking a grid variant, with the default preserving existing Home/placeholder behaviour exactly.
  • stateIn + WhileSubscribed(300ms) means tab round-trips don't re-flash the Loading spinner — and the contentKey on AnimatedContent correctly keeps the grid stable across podcast list updates.
  • Podcast being a data class means Loaded(sorted) dedupes in the StateFlow, so unrelated Room emissions don't recompose the grid.
  • Deviations (pinned title, no-op tap, no analytics) are documented with rationale rather than left to be discovered in review.
  • Previews for both states, and the tv module's existing test infra is used properly.

Note: I reviewed statically — I didn't run :tv:testDebugUnitTest or spotlessCheck locally, and the crash in item 2 is reasoned from the FocusRequester contract rather than reproduced on a device.

--- • Branch: feat/tv-your-podcasts

@sztomek

sztomek commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

@claude review again pls

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


Code Review (re-run after Address PR comments)

  • Gather context (diff vs origin/feat/tv-up-next)
  • Re-check previous review items
  • Review current state of screen + ViewModel + test
  • Post review

Both blockers from the last pass are properly fixed, and the fixes are the right shape rather than the minimum that silences the symptom. Nothing blocking left — the rest below is nits and one follow-up suggestion.


Previous items — status

# Item Status
1 Sorting on the main thread @DefaultDispatcher injected + .flowOn(defaultDispatcher) before stateIn (TvYourPodcastsViewModel.kt:23,37). Injecting the dispatcher rather than hardcoding Dispatchers.Default also makes the test deterministic — nice.
2 FocusRequester crash on off-screen item rememberLazyGridState() hoisted, lastFocusedUuid replaces the index, and onEnter only requests focus when the target index is in visibleItemsInfo, else falls back to the first visible item (TvYourPodcastsScreen.kt:130-146). The index >= 0 guard on indexOfFirst is there too.
3 Empty-state triplication ⬜ Not done — still three copies (see nit A).
4 Index vs uuid identity ✅ Covered by the item-2 fix.
5 Empty state steals focus ⬜ Kept, consistent with UpNextEmpty. Fine as a deliberate choice.
6 String naming tv_your_podcasts_empty_* (strings.xml:274-276).
7 imageModifier default ✅ Now Modifier, with Modifier.width(123.dp) pushed to the call sites.
8 Heading reuses tab string ⬜ Kept. Fine.
9 GRID_COLUMNS placement ⬜ Still at :196, between the composables and the previews. Cosmetic.
10 Tests Loaded → Empty transition test added (TvYourPodcastsViewModelTest.kt:60-74). No Loading preview (nit D).

One correction to my own item 1: I suggested decorate-sort-undecorate via PodcastsSortType.cleanStringForSort — that helper is private (PodcastsSortType.kt:16), so it isn't reachable without widening its visibility. compareBy still re-runs lowercase + regex + removeAccents() on every comparison, but it's off the main thread now, which was the part that mattered. Not worth exposing internals for.


Nits (all optional)

A. The empty state is now three identical composables. TvYourPodcastsEmpty (TvYourPodcastsScreen.kt:156-194) and UpNextEmpty (TvUpNextScreen.kt:182-218) are byte-identical apart from the three string ids; TvPlaylistsEmpty (TvPlaylistsScreen.kt:269-298) is the same shape minus the focusRequester and the subtitle widthIn. component/ is the obvious home for a TvEmptyState(title, subtitle, actionLabel, onAction) — and it would incidentally settle the focus-stealing divergence in one place. Happy to see it as a separate cleanup PR.

Fix this →

B. Modifier.width(123.dp) is now repeated at four call sitesTvHomeScreen.kt:150, TvTabPlaceholder.kt:83,97, and the TvPodcastTile preview. Moving it out of the default was the right call, but a named constant (TvPodcastTileDefaults.RowImageWidth, or a size: Dp param) would keep the magic number in one place and make the sharp edge discoverable — with the default now plain Modifier, a caller who forgets imageModifier gets a zero-width tile rather than a wrong-but-visible one.

C. TvPlaylistsScreen.kt:189-201 still has the pattern you just fixed here (rememberSaveable(playlists) index + unguarded requestFocus()). It's latently safe there because a 3-column playlist grid doesn't scroll, but now the two siblings diverge and the next grid will get copied from whichever one the author opens first. Worth either back-porting the uuid+visibility approach or extracting it as a rememberGridFocusRestorer(...) helper in component/ — as a follow-up, not in this PR.

D. No Loading preview to go with the grid/empty ones — one more @Preview with TvYourPodcastsUiState.Loading for the checklist item.

E. TvScaffold.kt:46-51navigateToDiscover is allocated inside the tab-content lambda, so it's recreated for every tab including the ones that don't take it. Deduplicating the Home/UpNext copies was a good change; hoisting the val above the TvScaffoldContent( call would be marginally tidier.


What's good in the new revision

  • @DefaultDispatcher injection over a hardcoded dispatcher — testable, and consistent with the repo's DispatcherModule.
  • The onEnter fix handles both failure modes (missing uuid → -1, and present-but-scrolled-out) with a sensible fallback rather than a try/catch.
  • rememberSaveable on the uuid is no longer keyed on the list at all, so focus memory now survives sync churn and process death.
  • The new Loaded → Empty test covers the transition the contentKey/AnimatedContent path depends on.

Reviewed statically: ./gradlew :tv:testDebugUnitTest and spotlessCheck need approval in this environment, so I couldn't run them — CI still needs to be the source of truth there.

--- • Branch: feat/tv-your-podcasts

@sztomek
sztomek force-pushed the feat/tv-your-podcasts branch from 7705ebe to ff84572 Compare August 3, 2026 10:24
@dangermattic

Copy link
Copy Markdown
Collaborator
1 Warning
⚠️ This PR is assigned to the milestone 8.18. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

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.

3 participants