Skip to content

[TV] Add folder support to the Your Podcasts tab - #5681

Merged
sztomek merged 3 commits into
mainfrom
feat/tv-podcast-folders
Aug 4, 2026
Merged

[TV] Add folder support to the Your Podcasts tab#5681
sztomek merged 3 commits into
mainfrom
feat/tv-podcast-folders

Conversation

@sztomek

@sztomek sztomek commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Description

Adds folder support to the TV Your Podcasts tab, mirroring the Apple TV FolderCardView / FolderDetailView. Follow-up to the podcasts grid (#5680).

  • Grid: the top-level grid now shows folders alongside loose podcasts. Folders render as a square card in the folder's colour with a 2×2 grid of the folder's top-4 podcast covers and the folder name at the bottom — the same layout tvOS's FolderCardView draws (covers top, name bottom, 12dp corners).
  • Folder detail: selecting a folder opens a detail screen — the folder name over a 6-column grid of that folder's podcasts, matching tvOS FolderDetailView. Back (or the empty-state button) returns to the grid.
  • Data: the view model reuses FolderManager.getHomeFolder() (folders + podcasts-not-in-a-folder) and re-queries it reactively whenever folders or subscriptions change, re-sorting the items A→Z with PodcastsSortType.NAME_A_TO_Z to match tvOS's titleAtoZ. Folder covers and detail podcasts come from FolderManager.findFolderPodcastsSorted().
  • Colour: the card resolves the folder colour through the Compose theme (LocalColors.current.colors.getFolderColor(...)), so it uses the TV app's Extra Dark palette — the analog of tvOS AppTheme.folderColor(colorInt:).
  • Reuse: extracted the title + focus-restoring 6-column grid into a shared TvPodcastGridScaffold, now used by both the top-level grid and the folder detail. The new TvFolderCard reuses the existing TvTile + TvArtworkImage (whose placeholder already matches tvOS's empty-cover fill).
  • Empty folder: "Your folder is empty" with an OK button that returns to the grid, matching tvOS's ContentUnavailableView.

Fixes PCDROID-697 https://linear.app/a8c/issue/PCDROID-697/folder-support.
Designs: Ftk3KwnfqaK4g57yCN63p0-fi-3246_3336.

Stacked on feat/tv-your-podcasts (#5680) — merge bottom-up.

Testing Instructions

  1. Install the TV app (./gradlew :tv:installDebugProd) and sign in with an account that has at least one folder with podcasts and some podcasts outside folders.
  2. Open the Your Podcasts tab — verify folders and loose podcasts are interleaved A→Z; each folder card shows the folder colour, up to four covers in a 2×2 grid, and the folder name.
  3. Select a folder — verify the detail screen opens with the folder name and a grid of that folder's podcasts, and the first tile is focused. Press Back — verify it returns to the grid.
  4. Open a folder that has no podcasts (create one in the mobile app) — verify "Your folder is empty" with a focused OK button that returns to the grid.
  5. Add/remove a folder or move a podcast in/out of a folder on another device — re-open the tab and verify the grid reflects the change.

Screenshots or Screencast

Screen_recording_20260729_190824.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 (extended 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)

@dangermattic

dangermattic commented Jul 29, 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-podcast-folders branch 3 times, most recently from 2c70c60 to c7aa413 Compare July 29, 2026 16:09
@sztomek sztomek added this to the 8.18 milestone Jul 29, 2026
@sztomek sztomek added [Area] TV [Type] Feature Adding a new feature. labels Jul 29, 2026
@sztomek
sztomek marked this pull request as ready for review July 29, 2026 17:09
@sztomek
sztomek requested a review from a team as a code owner July 29, 2026 17:09
@sztomek
sztomek requested review from geekygecko and removed request for a team July 29, 2026 17:09
@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 5m 53s —— View job


Code review

  • Read the PR diff against feat/tv-your-podcasts
  • Review new components (TvFolderCard, TvFolderDetailScreen)
  • Review screen + view model changes
  • Review tests
  • Post review

Nice change overall — the grid scaffold extraction is a good call, sorting via PodcastsSortType.NAME_A_TO_Z.folderComparator is the right analog of tvOS titleAtoZ, the folder colour goes through the theme correctly (the TV root wraps AppTheme(EXTRA_DARK) in TvOnboardingNavHost.kt:21, so LocalColors resolves to the Extra Dark palette as claimed), and the new UI is preview-covered. Comments below, roughly in priority order.


1. Folder covers never refresh — the grid is reactive, the cards aren't

FolderGridItem keys the cover load on folder.uuid only:

// TvYourPodcastsScreen.kt:181-184
var coverUrls by remember(folder.uuid) { mutableStateOf(emptyList<String>()) }
LaunchedEffect(folder.uuid, getFolderCoverUuids) {
    coverUrls = getFolderCoverUuids(folder.uuid).map(PodcastImage::getMediumArtworkUrl)
}

The view model re-runs getHomeFolder() when folders or subscriptions change, but a folder's uuid never changes, so the effect never re-runs. Move a podcast into/out of a folder (or let a sync do it) while the tab is open and the 2×2 artwork stays stale, even though the surrounding grid did update. TvFolderDetailScreen.kt:197-201 has the same one-shot shape — the detail list is a snapshot for the lifetime of the screen. Testing step 5 ("re-open the tab") quietly documents this.

2. Data loading via suspend (String) -> … params deviates from the module's own pattern

Both new lambdas (getFolderCoverUuids, getFolderPodcasts) push data access into composables. The TV module already established two conventions for exactly these two cases:

  • Detail screen → its own assisted-injected VM: TvPlaylistDetailsScreen.kt:73-76 does hiltViewModel<TvPlaylistDetailsViewModel, Factory>(key = playlistUuid, creationCallback = { it.create(playlistUuid, playlistType) }) and exposes uiState: StateFlow<…>. A TvFolderDetailViewModel(@Assisted folderUuid) would be reactive, testable, and drop the Loading/Empty/Loaded state juggling from the composable.
  • Per-card data → a flow from the VM: TvPlaylistsViewModel.kt:31-41 exposes getArtworkUuidsFlow(uuid): StateFlow<List<String>?> + refreshArtworkUuids(uuid).

Better still for the covers: enrich in TvYourPodcastsViewModel so the grid items arrive complete. Mobile already does this — PodcastsViewModel.kt:105-122 builds FolderItem.Folder(folder, podcasts) from observeFolders().flatMapLatest { … podcastManager.observePodcastsSortedByUserChoice(folder) … }. That fixes #1, removes the two lambda params (the previews currently pass { emptyList() }, so they can never show real covers), and removes the empty-placeholder pop-in on first paint.

Worth a comment either way: FolderManagerImpl.getHomeFolder() always returns FolderItem.Folder(podcasts = emptyList()) (FolderManagerImpl.kt:179), which is the non-obvious reason the extra per-folder query exists at all.

Fix this →

3. getHomeFolder() re-runs on every podcasts table write

// TvYourPodcastsViewModel.kt:28-33
combine(folderManager.observeFolders(), podcastManager.findSubscribedFlow()) { _, _ -> }
    .mapLatest { folderManager.getHomeFolder()… }

findSubscribedFlow() is a Room flow, so it re-emits the full podcast list on any write to podcasts (sync, settings, playback bookkeeping), and each emission runs getHomeFolder() (2+ queries). Consider .distinctUntilChanged() on a cheap projection of what you actually care about, e.g. map { list -> list.map { it.uuid to it.folderUuid } }.distinctUntilChanged(), before triggering the re-query.

4. Returning from a folder loses the grid's focus and scroll position

Swapping the whole subtree in TvYourPodcastsScreen.kt:74-94 tears down the grid, and nothing wraps the branches in a SaveableStateHolder, so lastFocusedIndex (rememberSaveable inside TvPodcastGridScaffold) and the internal LazyGridState are both discarded. Back from a folder lands on tile 0 at the top of the list rather than on the folder card you came from — tvOS restores it. Hoisting lastFocusedIndex + a LazyGridState above the if/else, or wrapping both branches in rememberSaveableStateHolder().SaveableStateProvider(key), would fix it. (TvPlaylistsScreen has the same shape, so this could be a shared follow-up rather than blocking here.)

5. TvPodcastGridScaffold focus effect

// TvYourPodcastsScreen.kt:211-215
if (autoFocusFirstItem) {
    LaunchedEffect(focusRequesters) { focusRequesters.firstOrNull()?.requestFocus() }
}
  • focusRequesters is re-created whenever itemKeys.size changes, so the effect re-fires and yanks focus back to tile 0 on any count change. Not reachable today (detail data is a snapshot), but it becomes a bug the moment Update issue templates #1/Updating the issue URL #2 make it reactive. LaunchedEffect(Unit) matches the rest of the module (TvUpNextScreen.kt:107, TvPlaylistDetailsScreen.kt:312).
  • Usual caveat: requestFocus() throws if the target hasn't been placed yet. The existing usages are on LazyColumns and behave; this is the first on a LazyVerticalGrid behind an AnimatedContent fade, so worth a device check with a large folder / slow load.

6. TvFolderCard nits

  • maxLines = 1 with no overflow (TvFolderCard.kt:99-108) clips long folder names mid-glyph. The shared FolderImage uses TextOverflow.Ellipsis — same here, plus softWrap = false.
  • The layout ratios at TvFolderCard.kt:74-77 and :83/:106 (0.32f, 0.024f, 0.096f, 0.064f) are unexplained magic numbers. FolderImage.kt:51-52 names its equivalents (IMAGE_SIZE_RATIO = 38f / 120f, PADDING_IMAGE_RATIO), which also documents the design spec they came from — worth copying that style. The one absolute value in the mix (padding(horizontal = 16.dp)) is inconsistent with the rest.

7. Empty state: copy + duplication

  • tv_folder_empty_message — "Edit your folder in the mobile app, they'll be waiting here when you're done." — "they" has no antecedent (the subject is "your folder") and it's a comma splice. This goes to GlotPress, so it's cheaper to tighten now: e.g. "Add podcasts to this folder in the mobile app and they'll show up here."
  • TvFolderDetailEmpty (TvFolderDetailScreen.kt:258-296) is a near-verbatim copy of TvYourPodcastsEmpty (TvYourPodcastsScreen.kt:250-287), and there are two more instances in TvPlaylistsScreen.kt:291 and TvUpNextScreen.kt:209. Since this PR is already in extract-the-shared-bits mode, a TvEmptyState(title, message, actionLabel, onAction) would pay for itself.

8. Placement

TvPodcastGridScaffold is internal and shared by two screens but lives inside TvYourPodcastsScreen.kt. Other shared TV UI lives under component/ — moving it there (or at least its own file) makes it discoverable.

9. Tests

Good that the existing cases were carried over. Gaps:

  • The headline VM behaviour — re-querying when folders change — isn't covered. observeFolders() is stubbed with flowOf(emptyList()), so the reactive path never fires more than once. A MutableSharedFlow<List<Folder>> plus a second emission with a changed homeFolder would assert it.
  • folderCoverUuids() is untested, including the take(4) cap, and folderPodcasts() is untested.
  • createViewModel() had to become suspend just to stub a suspend function; folderManager.stub { onBlocking { getHomeFolder() } doAnswer { homeFolder } } keeps it a plain function.
  • homeFolder = emptyList() in the first test is redundant — it's already the initial value.

Minor

  • OpenedFolder caches the folder name, so a rename while the detail is open shows the old title. Matches OpenedPlaylist, so consistent; just noting it.
  • TvPodcastTile(onClick = {}) in the detail grid is a no-op, same as the top-level grid — assuming podcast detail is a later PR.

Nothing here is a correctness blocker; #1 is the one I'd want resolved before merge (or explicitly deferred), and #2 is the cheapest way to get there.

CI: 6 workflows passed, 0 failed. I didn't run Gradle locally (build/lint run on Buildkite).
· branch feat/tv-podcast-folders

@sztomek
sztomek force-pushed the feat/tv-podcast-folders branch from c7aa413 to 2929681 Compare July 30, 2026 08:03
@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 4m 18s —— View job


Code review (round 2)

  • Diff 2929681 against the previously reviewed state
  • Re-check each prior finding
  • Review the new/changed code fresh
  • Post review

2929681 addressed most of the last round. The grid covers are now enriched in the view model, so the 2×2 artwork is reactive — the main thing I wanted resolved. Below: status of the old points, then what's new.


Status of previous findings

# Finding Status
1 Folder covers never refresh ✅ grid fixed (enriched in VM) — detail screen still one-shot
2 Data loading via suspend (String) -> … 🟡 getFolderCoverUuids gone; getFolderPodcasts remains
3 getHomeFolder() on every podcasts write ✅ projection + distinctUntilChanged — but see A below
4 Focus/scroll lost returning from a folder ❌ not addressed
5 LaunchedEffect(focusRequesters) ✅ now LaunchedEffect(Unit)
6 TvFolderCard nits 🟡 overflow fixed, ratios named — one naming slip, 16.dp still absolute
7 Empty-state copy / duplication ✅ copy fixed · ❌ still 4 copies of the empty state
8 Scaffold placement ✅ moved to component/TvPodcastGridScaffold.kt
9 Tests ✅ reactive + enrichment cases added, createViewModel() no longer suspend

A. Folder contents are sorted by the mobile sort setting, not A→Z

This is the one I'd want a decision on. findFolderPodcastsSorted() honours folder.podcastsSortType (FolderManagerImpl.kt:199-210), which is whatever the user picked in the mobile app — Date Added, Episode Release Date, Recently Played. So:

  • Covers (TvYourPodcastsViewModel.kt:43TvYourPodcastsScreen.kt:154) show the first four in that order.
  • Detail grid (TvFolderDetailScreen.kt:57) is ordered that way too.

Meanwhile the top-level grid is deliberately forced to NAME_A_TO_Z "to match tvOS's titleAtoZ" (TvYourPodcastsViewModel.kt:47). So a folder whose mobile sort is "Date Added" renders inside a strictly A→Z grid with non-alphabetical contents. If tvOS's FolderDetailView uses titleAtoZ, the folder contents should be forced the same way for consistency; if it intentionally honours the folder's own sort, then the top-level grid arguably should too. Right now it's split.

B. The enrichment is N+1, and getHomeFolder() does work that's then thrown away

// TvYourPodcastsViewModel.kt:40-47
val items = folderManager.getHomeFolder()
    .map { item ->
        when (item) {
            is FolderItem.Folder -> item.copy(podcasts = folderManager.findFolderPodcastsSorted(item.folder.uuid))
            …

Two costs stacked here:

  1. getHomeFolder() branches on settings.podcastsSortType.value (FolderManagerImpl.kt:158-176) — if the user's mobile sort is Episode Release Date or Recently Played it runs the expensive join (findPodcastsOrderByLatestEpisode / …RecentlyPlayedEpisode), and line 47 immediately discards that ordering by re-sorting A→Z.
  2. findFolderPodcastsSorted() is 2 more queries per folder (findByUuid + the podcast query). 20 folders → ~42 queries per emission, re-run on every folder/subscription change.

The whole thing collapses if you build the items from the two flows you already collect, with no suspend queries at all:

val uiState = combine(
    folderManager.observeFolders(),
    podcastManager.findSubscribedFlow(),
) { folders, podcasts ->
    val byFolder = podcasts.groupBy(Podcast::folderUuid)
    val items = folders.map { FolderItem.Folder(it, byFolder[it.uuid].orEmpty()) } +
        byFolder[null].orEmpty().map(FolderItem::Podcast)
    …items.sortedWith(PodcastsSortType.NAME_A_TO_Z.folderComparator)
}

findSubscribedFlow() already returns all subscribed podcasts (in-folder ones included) ordered by clean_title with a leading "the " stripped (PodcastDao.kt:46-59) — i.e. exactly the A→Z order this screen wants, so folder contents come out A→Z for free and A above resolves itself. observeFolders() already filters deleted = 0. That's zero DB round-trips per emission, no N+1, and the map { it.uuid to it.folderUuid } projection + distinctUntilChanged on line 36-38 becomes unnecessary (which also fixes the fact that the projection drops titles today, so a podcast renamed by sync won't re-sort the grid). It's also closer to mobile's PodcastsViewModel.

Trade-off: you lose getHomeFolder()'s "place the folder at the position of its most recent podcast" behaviour — irrelevant here since the list is re-sorted A→Z anyway.

Fix this →

C. The detail screen can now read from the grid state

Since every FolderItem.Folder already carries its full podcast list, TvFolderDetailScreen's getFolderPodcasts lambda (TvYourPodcastsScreen.kt:74, TvFolderDetailScreen.kt:51-59) is a second query for data the parent already has. Having OpenedFolder hold the uuid and resolving the item out of uiState (or passing the FolderItem.Folder through) would delete the lambda, the Loading/Empty/Loaded state machine in the composable, the duplicate query, and the remaining staleness (rename or membership change while the detail is open). This is the cheapest version of old point #2 now that the VM does the enrichment.

D. Focus/scroll still lost on the way back from a folder (old #4)

lastFocusedKey is now key-based and rememberSaveable — good — but TvYourPodcastsScreen.kt:66-85 still swaps the whole subtree, and rememberSaveable only survives that if something retains the saved value. Nothing wraps the branches in a SaveableStateHolder, so both lastFocusedKey (TvPodcastGridScaffold.kt:46) and the LazyGridState are discarded and Back lands on the first visible tile at the top. Fine as a shared follow-up with TvPlaylistsScreen — flagging that it's still open rather than fixed.

E. coverCornerRadius uses the spacing constant

// TvFolderCard.kt:59-60
val coverSpacing = cardWidth * COVER_SPACING_RATIO
val coverCornerRadius = cardWidth * COVER_SPACING_RATIO

Same number as before the refactor, so no visual change, but naming the constant made the coincidence look intentional — the corner radius now reads as being the spacing. COVER_CORNER_RADIUS_RATIO = 0.024f alongside keeps them independently tunable. (Also still padding(horizontal = 16.dp) at :91 amid four ratios.)

Minor

  • FOLDER_COVER_COUNT = 4 lives in TvYourPodcastsScreen.kt:202 but the 2×2 layout it must match is repeat(2)/repeat(2) in TvFolderCard.kt:68-70. Doing the take inside TvFolderCard would keep them from drifting.
  • tv_folder_empty_message ("Add podcasts to this folder in the mobile app and they'll show up here.") is very close to the existing podcasts_empty_folder_summary ("Add podcasts to your folder and they'll appear here.", strings.xml:617). If the "in the mobile app" hint isn't essential, reusing the existing string saves a translation round.
  • folderPodcasts() is still untested — moot if C lands.
  • Test name folder items are enriched with their cover podcasts slightly over-promises now that the take(4) cap lives in the composable; it asserts full enrichment.

Nothing here is a correctness blocker. A is the one that needs a call (behavioural, user-visible), B collapses A/#3 and a chunk of the code at once, C and D are the leftovers from last round.

CI: 6 workflows passed, 0 failed. I didn't run Gradle locally (build/lint/tests run on Buildkite).
· branch feat/tv-podcast-folders

@sztomek sztomek mentioned this pull request Jul 30, 2026
10 tasks
@sztomek
sztomek force-pushed the feat/tv-your-podcasts branch from 7705ebe to ff84572 Compare August 3, 2026 10:24
@sztomek
sztomek force-pushed the feat/tv-podcast-folders branch from 2929681 to a7952c8 Compare August 3, 2026 10:24
@sztomek
sztomek force-pushed the feat/tv-your-podcasts branch from ff84572 to a8f1255 Compare August 3, 2026 11:25
@sztomek
sztomek force-pushed the feat/tv-podcast-folders branch from a7952c8 to 71916e8 Compare August 3, 2026 11:26
@claude

claude Bot commented Aug 3, 2026

Copy link
Copy Markdown

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


Code review (round 3)

  • Diff the new push against the previously reviewed state
  • Re-check round-2 findings (A–E)
  • Review new/changed code fresh
  • Post review

First, a scoping note: the commit I reviewed in round 2 (2929681) is no longer reachable — the branch was rebased onto the updated base (a8f1255, which is where #5680 picked up my round-1 comments). So I re-read the whole PR fresh. As far as I can tell this push is the rebase only: every location round 2 referenced (TvYourPodcastsViewModel.kt:36-47, TvPodcastGridScaffold.kt:46, TvFolderCard.kt:59-60, TvFolderDetailScreen.kt:51-59, TvYourPodcastsScreen.kt:66-85, :202) resolves to identical code today. So A, B, C, D and E are all still open, plus the rebase surfaced one new thing.


New: the base branch now has a shared TvEmptyState, and this PR adds a third hand-rolled copy

a8f1255 on feat/tv-your-podcasts added component/TvEmptyState.kt (title + subtitle + filled button, widthIn(max = 400.dp), 12/32dp spacers) and converted TvPlaylistsScreen.kt:274 to it — that was round-1 point #7. This PR's TvYourPodcastsEmpty (TvYourPodcastsScreen.kt:163-200) and the new TvFolderDetailEmpty (TvFolderDetailScreen.kt:117-154) are line-for-line that component, sitting one directory away from it.

The only thing keeping them local is the FocusRequester on the button — which TvEmptyState doesn't have, and which means TvPlaylistsScreen's empty state currently ships with no focused control at all on a TV. One param fixes both:

fun TvEmptyState(
    title: String,
    subtitle: String,
    actionLabel: String,
    onAction: () -> Unit,
    modifier: Modifier = Modifier,
    autoFocusAction: Boolean = true,
) { … }

That deletes ~75 lines here and closes the gap on the playlists screen.

Fix this →


Still open from round 2

A. Folder contents use the mobile sort setting, not A→Z — the one that needs a product call. Re-verified: findFolderPodcastsSorted() branches on folder.podcastsSortType (FolderManagerImpl.kt:198-211), so the 2×2 covers (TvYourPodcastsViewModel.kt:43TvYourPodcastsScreen.kt:154) and the detail grid (TvFolderDetailScreen.kt:57) come out in whatever order the user picked on mobile, while the surrounding grid is force-sorted NAME_A_TO_Z (:47). If tvOS's FolderDetailView is titleAtoZ, force it here too.

B. Enrichment is N+1 and getHomeFolder()'s ordering work is thrown away — re-verified against FolderManagerImpl.kt:158-176: with a mobile sort of Episode Date or Recently Played, getHomeFolder() runs the expensive join and line 47 immediately discards that order. Plus 2 queries per folder from findFolderPodcastsSorted(). Building the items from the two flows you already collect (group findSubscribedFlow() by folderUuid) is zero round-trips, resolves A for free — PodcastDao.kt:52-58 already orders by clean_title with a leading "the " stripped — and lets the uuid to folderUuid projection at :36-38 go away (it currently drops titles, so a sync-side rename won't re-sort the grid).

C. TvFolderDetailScreen's getFolderPodcasts lambda (TvYourPodcastsScreen.kt:74) re-queries data the parent already holds — every FolderItem.Folder in uiState now carries its full podcast list. Passing the item through deletes the lambda, the Loading/Empty/Loaded state machine, and the remaining staleness while the detail is open.

D. Focus/scroll still lost coming back from a folder — TvYourPodcastsScreen.kt:66-85 swaps the subtree with nothing retaining lastFocusedKey / LazyGridState. Fine as a shared follow-up with TvPlaylistsScreen; flagging that it's unaddressed.

E. TvFolderCard.kt:59-60coverCornerRadius = cardWidth * COVER_SPACING_RATIO. Naming the constant made the shared value read as intentional; a separate COVER_CORNER_RADIUS_RATIO = 0.024f keeps them tunable. padding(horizontal = 16.dp) at :91 is still the one absolute among four ratios.

Minor

  • TvPodcastGridScaffold.kt:10 imports androidx.compose.foundation.lazy.grid.items, but the call at :72 is the LazyGridScope.items(count, key) member — the extension import is unused. It's new (the base used itemsIndexed), and ktlint's name-based no-unused-imports won't catch it.
  • FOLDER_COVER_COUNT = 4 (TvYourPodcastsScreen.kt:202) still lives away from the repeat(2)/repeat(2) it has to match (TvFolderCard.kt:68-70).
  • TvFolderCard reads LocalColors (:42), so it silently requires an ambient AppTheme. WearColors.kt:17 shows the module-local alternative (ThemeExtraDarkColors.getFolderColor(id)); a TvColors.getFolderColor(id) would make the card self-contained. Works today either way.
  • folderPodcasts() still untested (moot if C lands); test name folder items are enriched with their cover podcasts still over-promises — the take(4) cap lives in the composable, not the VM.
  • remember(itemKeys.size) for the focus requesters (TvPodcastGridScaffold.kt:46) rebuilds the whole list on any count change. Benign today, but it's the kind of thing that bites once the grid updates under a focused tile.

Same conclusion as last round: no correctness blockers. A needs a decision, B collapses A and the query cost together, and the new TvEmptyState point is now a cheap win since the component landed on the base branch.

CI: 2 passed, 0 failed, 1 in progress (this job). I didn't run Gradle locally — build/lint/tests run on Buildkite.
· branch feat/tv-podcast-folders

@sztomek
sztomek force-pushed the feat/tv-your-podcasts branch from a8f1255 to d1cb9cd Compare August 3, 2026 11:57
@sztomek
sztomek force-pushed the feat/tv-podcast-folders branch from 71916e8 to 34fcffa Compare August 3, 2026 11:57
@claude

claude Bot commented Aug 3, 2026

Copy link
Copy Markdown

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


Code review (round 4)

  • Diff the new push against the previously reviewed state
  • Re-check round-3 findings (A–E + TvEmptyState)
  • Review new/changed code fresh
  • Post review

This push is a rebase, not a code change. The base moved (origin/feat/tv-your-podcasts is now d1cb9cd) and the PR's three commits were replayed on top. git diff origin/feat/tv-your-podcasts...HEAD is byte-identical to round 3 — same 8 files, same +592/−82, and the tip commit is still 34fcffa ("Address PR comments", authored Jul 30). I re-verified every location and every claim below against the current tree rather than trusting the previous round.

So everything from round 3 is still open. Compressed re-statement, no new findings:

Finding Re-verified at
A Folder contents use the mobile sort setting, not A→Z — the covers and the detail grid come out in whatever the user picked on mobile, inside a grid that's force-sorted NAME_A_TO_Z. Needs a product call. FolderManagerImpl.kt:198-211 branches on folder.podcastsSortType; TvYourPodcastsViewModel.kt:43TvYourPodcastsScreen.kt:154; TvFolderDetailScreen.kt:57; forced sort at TvYourPodcastsViewModel.kt:47
B Enrichment is N+1 and getHomeFolder()'s ordering work is discarded. Building the items from the two flows you already collect (group findSubscribedFlow() by folderUuid) is zero DB round-trips and resolves A for free. FolderManagerImpl.kt:158-176 runs the expensive join for Episode-Date / Recently-Played then line 47 throws that order away; findFolderPodcastsSorted = 2 queries per folder; PodcastDao.kt:53-56 already orders by clean_title with a leading "the " stripped, i.e. the A→Z order this screen wants
C getFolderPodcasts re-queries data the parent already holds — every FolderItem.Folder in uiState carries its full podcast list. Passing the item through deletes the lambda, the Loading/Empty/Loaded machine, and the staleness while the detail is open. TvYourPodcastsScreen.kt:74, TvFolderDetailScreen.kt:51-59
D Focus/scroll still lost coming back from a folder — the subtree is swapped with nothing retaining lastFocusedKey / LazyGridState. Fine as a shared follow-up with TvPlaylistsScreen. TvYourPodcastsScreen.kt:66-85, TvPodcastGridScaffold.kt:46-47
E coverCornerRadius = cardWidth * COVER_SPACING_RATIO — naming the constant made the shared value read as intentional; a separate COVER_CORNER_RADIUS_RATIO = 0.024f keeps them tunable. padding(horizontal = 16.dp) at :91 is still the one absolute among four ratios. TvFolderCard.kt:58-60, :91

TvEmptyState — re-verified on the new base: component/TvEmptyState.kt exists and TvPlaylistsScreen.kt:274 uses it. This PR's TvFolderDetailEmpty (TvFolderDetailScreen.kt:117-154) is line-for-line that component plus a FocusRequester, and TvYourPodcastsEmpty (TvYourPodcastsScreen.kt:163-200, pre-existing on the base) is too. Adding autoFocusAction: Boolean = true to TvEmptyState deletes ~75 lines here and gives the playlists empty state a focused control, which it currently lacks. (UpNextEmpty in TvUpNextScreen.kt:182 is the third hand-rolled copy, out of scope.)

Fix this →

Minor, also still open: unused androidx.compose.foundation.lazy.grid.items import at TvPodcastGridScaffold.kt:10 (the call at :72 is the LazyGridScope.items(count, key) member, so the extension import is dead — ktlint's name-based no-unused-imports won't catch it) · FOLDER_COVER_COUNT = 4 (TvYourPodcastsScreen.kt:202) lives away from the repeat(2)/repeat(2) it must match (TvFolderCard.kt:68-70) · TvFolderCard reads LocalColors so it silently requires an ambient AppTheme (a TvColors.getFolderColor(id) would make it self-contained; works today) · folderPodcasts() untested — moot if C lands · the test name folder items are enriched with their cover podcasts over-promises, since the take(4) cap lives in the composable.

Same conclusion as rounds 2 and 3: no correctness blockers. A needs a decision, B collapses A and the query cost together, and C/D/TvEmptyState are cheap cleanups. If you'd rather ship as-is and file A–D as follow-ups, say so and I'll stop re-raising them.

CI: 5 passed, 0 failed, 1 in progress (this job). I didn't run Gradle locally — build/lint/tests run on Buildkite.
· branch feat/tv-podcast-folders

@wpmobilebot wpmobilebot modified the milestones: 8.18, 8.19 Aug 3, 2026
@wpmobilebot

Copy link
Copy Markdown
Collaborator

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

@sztomek
sztomek force-pushed the feat/tv-your-podcasts branch from d1cb9cd to 38c5840 Compare August 3, 2026 13:21
@sztomek
sztomek force-pushed the feat/tv-podcast-folders branch from 34fcffa to c9cb247 Compare August 3, 2026 13:22
@claude

claude Bot commented Aug 3, 2026

Copy link
Copy Markdown

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


Code review (round 5)

  • Diff the new push against the previously reviewed state
  • Re-check round-4 findings (A–E + TvEmptyState)
  • Review new/changed code fresh
  • Post review

This push is another rebase, not a code change. The base moved again (origin/feat/tv-your-podcasts is now 38c5840) and the three commits were replayed (c9cb247 / 9fb1a00 / c62027e, author dates still Jul 29–30). git diff origin/feat/tv-your-podcasts...HEAD is still 8 files, +592/−82, and I re-read every file and re-verified every claim below against the current tree rather than trusting the previous round — all line references still resolve to identical code.

Everything from round 4 is still open. Short re-statement, plus one new minor observation:

Finding Re-verified at
A Folder contents use the mobile sort setting, not A→Z — covers and the detail grid come out in whatever the user picked on mobile, inside a grid force-sorted NAME_A_TO_Z. Needs a product call. FolderManagerImpl.kt:198-211 branches on folder.podcastsSortType; TvYourPodcastsViewModel.kt:43TvYourPodcastsScreen.kt:154; TvFolderDetailScreen.kt:57; forced sort at TvYourPodcastsViewModel.kt:47
B Enrichment is N+1 and getHomeFolder()'s ordering work is discarded. Building the items from the two flows you already collect (group findSubscribedFlow() by folderUuid) is zero DB round-trips and resolves A for free. FolderManagerImpl.kt:158-176 — confirmed again: Episode-Date / Recently-Played run the expensive join at :162-163, then :174's comparator is skipped and TvYourPodcastsViewModel.kt:47 re-sorts anyway; findFolderPodcastsSorted = 2 queries per folder
C getFolderPodcasts re-queries data the parent already holds — every FolderItem.Folder in uiState carries its full podcast list. Passing the item through deletes the lambda, the Loading/Empty/Loaded machine, and the staleness while the detail is open. TvYourPodcastsScreen.kt:74, TvFolderDetailScreen.kt:51-59
D Focus/scroll still lost coming back from a folder — the subtree is swapped with nothing retaining lastFocusedKey / LazyGridState. Fine as a shared follow-up with TvPlaylistsScreen. TvYourPodcastsScreen.kt:66-85, TvPodcastGridScaffold.kt:44-46
E coverCornerRadius = cardWidth * COVER_SPACING_RATIO — the shared value reads as intentional now that the constant is named; a separate COVER_CORNER_RADIUS_RATIO = 0.024f keeps them tunable. TvFolderCard.kt:58-60

TvEmptyState — re-verified on the new base: component/TvEmptyState.kt exists (title / subtitle / filled button, widthIn(max = 400.dp), 12+32dp spacers) and has no FocusRequester. TvFolderDetailEmpty (TvFolderDetailScreen.kt:117-154) and TvYourPodcastsEmpty (TvYourPodcastsScreen.kt:163-200) are line-for-line that component plus a focus requester. Adding autoFocusAction: Boolean = true deletes ~75 lines here and gives the playlists empty state a focused control, which it currently lacks. (Only cosmetic difference to reconcile: TvEmptyState styles the button label with TvTextStyles.ModalButtonLabel, these two don't.)

Fix this →

New this round (minor)

TvFolderDetailScreen.kt:56 keys its effect on the lambda itself:

LaunchedEffect(folderUuid, getFolderPodcasts) { … }

getFolderPodcasts is viewModel::folderPodcasts (TvYourPodcastsScreen.kt:74), re-created on every recomposition of TvYourPodcastsScreen — which recomposes on every uiState emission, including while the detail screen is on top. Whether the effect restarts then depends on whether that bound suspend reference has structural equality (Kotlin 2.x compiles references via invokedynamic, which doesn't), so this either re-queries the folder on every grid update or never re-queries at all — and it isn't obvious from the call site which. folderUuid alone as the key would at least make the intent explicit; C removes the question entirely.

Also, TvTextStyles.FolderCardTitle is a fixed 14.sp while every other dimension in TvFolderCard scales off cardWidth — same category as the leftover padding(horizontal = 16.dp) at :91. Fine at one card size; worth knowing if the card ever gets resized.

Also still open (minor): unused androidx.compose.foundation.lazy.grid.items import at TvPodcastGridScaffold.kt:10 — the call at :72 is the LazyGridScope.items(count, key) member, so the extension import is dead and ktlint's name-based no-unused-imports won't catch it · FOLDER_COVER_COUNT = 4 (TvYourPodcastsScreen.kt:202) lives away from the repeat(2)/repeat(2) it must match (TvFolderCard.kt:68-70) · TvFolderCard reads LocalColors (:42) so it silently requires an ambient AppTheme · folderPodcasts() untested — moot if C lands · the test name folder items are enriched with their cover podcasts (TvYourPodcastsViewModelTest.kt:98) over-promises, since the take(4) cap lives in the composable.


Same conclusion as rounds 2–4: no correctness blockers. A needs a decision, B collapses A and the query cost together, C/D/TvEmptyState are cheap cleanups. Since the last two pushes have been rebases with no code change, I'll stop re-raising A–E on future rebases unless you ask — ping me when there's an actual code change to look at, or tell me you're filing them as follow-ups and I'll drop them.

CI: 2 passed, 0 failed, 1 in progress (this job). I didn't run Gradle locally — build/lint/tests run on Buildkite.
· branch feat/tv-podcast-folders

@sztomek
sztomek force-pushed the feat/tv-your-podcasts branch from 38c5840 to 0bdff36 Compare August 3, 2026 16:06
@sztomek
sztomek force-pushed the feat/tv-podcast-folders branch from c9cb247 to e064c5d Compare August 3, 2026 16:15
@claude

claude Bot commented Aug 3, 2026

Copy link
Copy Markdown

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


Code review (round 6)

  • Diff the new push against the previously reviewed state
  • Re-check round-5 findings (A–E + TvEmptyState)
  • Review new/changed code fresh
  • Post review

Unlike the last two pushes, this one contains real changes. The tip commit was rewritten (c62027ee064c5d) on a new base (0bdff36), and the diff grew from +592/−82 to +598/−89. Two of round 5's minor findings are fixed, and the base absorbed one more. Everything else I re-verified against the current tree.


Fixed this round

  • LaunchedEffect(folderUuid, getFolderPodcasts)LaunchedEffect(folderUuid) (TvFolderDetailScreen.kt:56). The invokedynamic-equality question is gone; the intent now reads correctly.
  • Unused androidx.compose.foundation.lazy.grid.items import removed from TvPodcastGridScaffold.kt. The call at :71 is the LazyGridScope.items(count, key) member, so nothing needed importing — confirmed the import list is clean now.
  • TvYourPodcastsEmpty is gone — the rebased base already routes TvYourPodcastsUiState.Empty through the shared component/TvEmptyState.kt (TvYourPodcastsScreen.kt:104-110). Not this PR's change, but it closes half of the duplication point.

The one leftover on the shared empty state

TvFolderDetailEmpty (TvFolderDetailScreen.kt:116-154) is now the only hand-rolled copy this PR adds, and it is line-for-line TvEmptyState — same ScreenTitle / bodyLarge + TextSecondary / widthIn(max = 400.dp) / 12dp + 32dp spacers / TvButtonDefaults.filledButtonColors() — plus a FocusRequester on the button and minus TvTextStyles.ModalButtonLabel on the label. Adding autoFocusAction: Boolean = true to TvEmptyState deletes ~38 lines here and gives the TvYourPodcastsScreen / TvPlaylistsScreen empty states a focused control, which they currently lack on a TV (nothing is focusable in TvEmptyState today, so the D-pad has nowhere to land).

Fix this →

Still open (A–E), re-verified against the current tree

Finding Re-verified at
A Folder contents use the mobile sort setting, not A→Z. findFolderPodcastsSorted() branches on folder.podcastsSortType, so the 2×2 covers and the detail grid come out in whatever the user picked on mobile, inside a grid force-sorted NAME_A_TO_Z. Needs a product call. FolderManagerImpl.kt:199-212; TvYourPodcastsViewModel.kt:43TvYourPodcastsScreen.kt:143; TvFolderDetailScreen.kt:57; forced sort at TvYourPodcastsViewModel.kt:47
B Enrichment is N+1 and getHomeFolder()'s ordering work is discarded. Re-confirmed: with a mobile sort of Episode-Date / Recently-Played, getHomeFolder() runs the expensive join at :162-163 and returns folderItems unsorted at :172, which :47 then re-sorts anyway; findFolderPodcastsSorted adds 2 queries per folder. Building the items from the two flows you already collect (group findSubscribedFlow() by folderUuid) is zero DB round-trips and resolves A for free, since PodcastDao already orders by clean_title with a leading "the " stripped. FolderManagerImpl.kt:158-176, :198-212; TvYourPodcastsViewModel.kt:34-47
C getFolderPodcasts re-queries data the parent already holds — every FolderItem.Folder in uiState carries its full podcast list. Passing the item through deletes the lambda, the Loading/Empty/Loaded machine, and the staleness while the detail is open. TvYourPodcastsScreen.kt:60, TvFolderDetailScreen.kt:51-59
D Focus/scroll still lost coming back from a folder — TvYourPodcastsScreen.kt:52-71 swaps the whole subtree with nothing retaining lastFocusedKey / LazyGridState. Fine as a shared follow-up with TvPlaylistsScreen. TvYourPodcastsScreen.kt:52-71, TvPodcastGridScaffold.kt:43-45
E coverCornerRadius = cardWidth * COVER_SPACING_RATIO — now that the constant is named, the shared value reads as intentional; a separate COVER_CORNER_RADIUS_RATIO = 0.024f keeps them tunable. TvFolderCard.kt:58-60

Minor, also still open

  • padding(horizontal = 16.dp) at TvFolderCard.kt:91 is the one absolute among four ratios, and TvTextStyles.FolderCardTitle is a fixed 14.sp while everything else scales off cardWidth. Fine at one card size.
  • FOLDER_COVER_COUNT = 4 (TvYourPodcastsScreen.kt:151) still lives away from the repeat(2)/repeat(2) it has to match (TvFolderCard.kt:68-70).
  • TvFolderCard reads LocalColors (:42), so it silently requires an ambient AppTheme. A TvColors.getFolderColor(id) would make it self-contained; works today.
  • The uuid to folderUuid projection (TvYourPodcastsViewModel.kt:36) drops titles, so a sync-side podcast rename won't re-sort the grid. (Folder renames do propagate — observeFolders() emits the changed entity.)
  • folderPodcasts() untested (moot if C lands); test name folder items are enriched with their cover podcasts (TvYourPodcastsViewModelTest.kt:98) still over-promises, since the take(4) cap lives in the composable.
  • TvPodcastGridScaffold.kt:47-50requestFocus() from LaunchedEffect(Unit) on a LazyVerticalGrid behind an AnimatedContent fade. Lazy children are composed during measure, so the requester may not be attached when the effect body runs; the module's other usages are on LazyColumns and behave, but this is the first grid — worth one device check with a large folder.

Same conclusion as rounds 2–5, minus the two items you fixed: no correctness blockers. A still needs a decision, B collapses A and the query cost together, C/D and the TvEmptyState consolidation are cheap cleanups. Happy to stop re-raising A–E if you're filing them as follow-ups — just say so.

CI: 2 passed, 0 failed, 1 in progress (this job). I didn't run Gradle locally — build/lint/tests run on Buildkite.
· branch feat/tv-podcast-folders

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

This has similar behaviour to what I mentioned on another screen. The scrollable content doesn't include the header and title. It might be nice to address that in a future PR.

}
}
}
}

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.

Can this use a common component?

@sztomek
sztomek force-pushed the feat/tv-your-podcasts branch from 0bdff36 to 1d5a68b Compare August 4, 2026 07:46
Base automatically changed from feat/tv-your-podcasts to main August 4, 2026 07:56
@sztomek
sztomek force-pushed the feat/tv-podcast-folders branch from e064c5d to f8974ec Compare August 4, 2026 08:05
@sztomek

sztomek commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main now that #5680 has merged. The rebase surfaced one conflict in TvYourPodcastsScreen.kt: this PR's e064 extracts the grid into component/TvPodcastGridScaffold.kt and deletes the local copy, while main had absorbed my #5680 runCatching focus-crash backstop. Resolved by taking the extraction and re-applying the runCatching guard to the extracted scaffold's onEnter so that fix isn't lost.

Treating round 6:

Fixed:

  • The TvEmptyState consolidation (the 'one leftover'). Added an autoFocusAction parameter to TvEmptyState and deleted the ~38-line hand-rolled TvFolderDetailEmpty, which was line-for-line the shared component minus ModalButtonLabel on the button. The folder-detail empty now goes through TvEmptyState(autoFocusAction = true) — so it keeps its focused OK button and picks up the correct button typography it was missing. I defaulted autoFocusAction = false to avoid changing the already-merged Your Podcasts / Playlists empty states; flipping their default to auto-focus is a reasonable separate UX call if you want the D-pad to land on the action there too.
  • Grid auto-focus crash guard. Wrapped the autoFocusFirstItem requestFocus() in TvPodcastGridScaffold in runCatching — same class as your 'worth a device check' note, and the same guard I added to onEnter, since a lazy grid child may not be attached when the effect runs.
  • compose:lambda-param-in-effect on getFolderPodcasts. ktlint (locally) flags the LaunchedEffect(folderUuid) that references the getFolderPodcasts lambda directly — the very change praised as a fix last round drops it as a key. Wrapped it in rememberUpdatedState so the effect stays keyed on folderUuid only while satisfying the rule. (Moot once C lands and the lambda goes away, but it was blocking spotless.)

Deferring as follow-ups — taking you up on the offer to stop re-raising A–E:

  • A (folder contents use the mobile sort, not A→Z) needs a product decision, so not mine to force here.
  • B (rebuild items from findSubscribedFlow() grouped by folderUuid — kills the N+1 and resolves A for free) is the strategic fix, but it decides A and is a real ViewModel rewrite; better as its own change.
  • C/D (pass the folder's podcast list through instead of the re-query lambda; retain focus/scroll returning from a folder) are the interrelated data-flow cleanups — C in particular deletes the whole Loading/Empty/Loaded machine here, worth doing deliberately with the TvPlaylistsScreen focus-restore work.
  • E and the smaller nits (FOLDER_COVER_COUNT placement, TvColors.getFolderColor, the title-rename re-sort) — filing alongside the above.

Validation: spotlessApply clean, :tv:compileDebugKotlin + :tv:testDebugUnitTest both green.

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown

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


Code review (round 7)

  • Diff the new push against the previously reviewed state
  • Verify the rebase conflict resolution kept the runCatching backstop from main
  • Review the three fixes (TvEmptyState consolidation, grid focus guard, rememberUpdatedState)
  • Post review

Thanks for the write-up — I'll take you up on it and drop A–E from here on; they're yours to file. This round is only the new code.

Rebase resolution checks out. origin/main's TvYourPodcastsGrid had the runCatching guard inside focusProperties { onEnter { … } }; the extracted component/TvPodcastGridScaffold.kt:63 carries it verbatim, and the local grid + its GRID_COLUMNS are gone from TvYourPodcastsScreen.kt with no leftovers. The #5680 fix survived.

The three fixes look right:

  • TvEmptyState(autoFocusAction) + deleting TvFolderDetailEmpty — the folder-detail empty now goes through the shared component and picks up ModalButtonLabel. This also answers @geekygecko's inline "Can this use a common component?" on TvFolderDetailScreen.kt.
  • rememberUpdatedState(getFolderPodcasts) with LaunchedEffect(folderUuid) (TvFolderDetailScreen.kt:58-62) — keyed on the uuid, reads the latest lambda, satisfies compose:lambda-param-in-effect. Correct.
  • Defaulting autoFocusAction = false is a fine call. One correction to my round-6 phrasing, since it bears on whether you'd ever flip it: I said TvEmptyState left "nothing focusable" — that was wrong. androidx.tv.material3.Button is D-pad reachable either way; the only difference is initial focus. So flipping the default is purely a nicety, not a fix.

1. TvFolderDetailScreen.kt has 13 imports orphaned by deleting TvFolderDetailEmpty

None of these names appear anywhere outside the import block now:

Column (:10) · Spacer (:11) · height (:14) · widthIn (:15) · Alignment (:23) · FocusRequester (:25) · focusRequester (:26) · TextAlign (:29) · dp (:32) · Button (:33) · Text (:35) · TvButtonDefaults (:43) · TvTextStyles (:45)

ktlint's no-unused-imports is name-based, and unlike the grid.items case from round 3 (where items did appear in the body as a member call, which is why the rule missed it) every one of these is a clean miss — so I'd expect spotlessCheck to fail on Buildkite. Since you reported spotlessApply clean, my guess is it ran before the TvFolderDetailEmpty deletion, or against a stale spotlessPreCommitFiles list. Worth a re-run. I couldn't run Gradle in this job (no permission), so I'm reasoning from the rule rather than an observed failure — and Buildkite isn't visible here (GitHub Actions shows only Validate Gradle Wrapper ×2 + this job).

Fix this →

2. The runCatching guards turn "crash" into "silently nothing focused"

Three sites now: TvEmptyState.kt:43, TvPodcastGridScaffold.kt:48 (autoFocusFirstItem), and the pre-existing onEnter at :63.

For onEnter this is exactly right — it fires on a user focus event, the grid is already placed, and a swallowed throw is a genuine edge case. For the two LaunchedEffect(Unit) sites it's doing something different: the failure mode it catches is the normal first-frame case (requester not yet attached), and there's no retry, so the outcome is that autoFocusFirstItem = true / autoFocusAction = true silently does nothing. That's testing step 3 ("the first tile is focused") and step 4 ("a focused OK button") quietly not holding.

Your earlier screencast shows focus landing, so on a fast local device the requester is evidently attached by the time the effect body runs — the guard is a backstop, not masking a live bug. But it's a backstop that fails silently precisely where it's most likely to trigger (a large folder, a cold image cache, the AnimatedContent fade). Waiting for placement instead of catching would make the intent hold:

if (autoFocusFirstItem) {
    LaunchedEffect(Unit) {
        snapshotFlow { gridState.layoutInfo.visibleItemsInfo.isNotEmpty() }.first { it }
        runCatching { focusRequesters.firstOrNull()?.requestFocus() }
    }
}

TvEmptyState has no lazy container, so Modifier.onPlaced { … } (or a one-shot awaitFrame()) is the equivalent there. Low priority — flagging that the guard changed the failure mode rather than removing it.

Fix this →

Minor

  • Preview gap. Deleting TvFolderDetailEmpty also deleted the only preview of the folder-detail empty state, and TvEmptyStatePreview (TvEmptyState.kt:77-93) uses the default autoFocusAction = false — so the new parameter has no preview coverage at all. A second TvFolderDetailPreview on TvFolderDetailUiState.Empty is one composable and keeps the checklist honest.
  • val focusRequester = remember { FocusRequester() } (TvEmptyState.kt:40) is allocated unconditionally and then only wired up when autoFocusAction is true. Harmless, just slightly odd to read next to the conditional modifier at :69.
  • Test coverage regression, PR-wide rather than this push: the old unsubscribing the last podcast falls back to the empty state case was replaced, and nothing now covers the Loaded → Empty transition — only the initial Loading → Empty. Cheap to add back onto the reactive test at TvYourPodcastsViewModelTest.kt:75 (set homeFolder = emptyList() and emit again).

Net: #1 is the one to action before merge (probable red spotless), #2 is a judgement call on the guards, everything else is optional. A–E dropped as agreed.

CI: 2 passed, 0 failed, 1 in progress (this job). Buildkite (build/lint/tests) isn't visible from here, and I couldn't run Gradle locally.
· branch feat/tv-podcast-folders

@sztomek
sztomek force-pushed the feat/tv-podcast-folders branch from f8974ec to bfa58dd Compare August 4, 2026 08:18
@sztomek

sztomek commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Both caught correctly — thank you, and you were right on the mechanism.

1. The 13 orphaned imports. Confirmed: all 13 had zero body references and were still in the file, so spotlessCheck would have failed on Buildkite. You nailed the cause — my spotlessApply gave a false clean: the first run aborted on the lambda-param-in-effect error before writing the import auto-fixes, and the re-run served a stale up-to-date result for the file. Removed all 13, and this time I verified with ./gradlew spotlessApply spotlessCheck --rerun-tasks (observed green, no stale caching) rather than trusting apply.

2. runCatching changed the failure mode on the two auto-focus sites. Agreed — on onEnter it's correct (user event, grid already placed), but on the two LaunchedEffect(Unit) sites it was swallowing the normal first-frame not-yet-attached case with no retry, so autoFocusFirstItem/autoFocusAction could silently no-op exactly where it matters (large folder, cold cache, the AnimatedContent fade). Switched both to wait for placement:

  • Grid: snapshotFlow { gridState.layoutInfo.visibleItemsInfo.isNotEmpty() }.first { it } before the requestFocus() (kept runCatching as a pure backstop behind the wait).
  • TvEmptyState: no lazy container, so a one-shot withFrameNanos {} before the requestFocus().

Validation this round: spotlessApply + spotlessCheck --rerun-tasks green, :tv:compileDebugKotlin + :tv:testDebugUnitTest green.

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown

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


Code review (round 8)

  • Diff the new push against the previously reviewed state
  • Verify the 13 orphaned imports are gone
  • Review the two placement-wait auto-focus changes
  • Post review

Both round-7 items are fixed, and the mechanism you described matches what's in the tree. A–E stayed dropped as agreed, so this round is only the new code. Nothing blocking.


Verified fixed

1. Orphaned imports. All 13 are gone from TvFolderDetailScreen.kt — the import block is now 33 lines and every name resolves to a body reference (Box/background/TvColors/Theme/AppTheme in the preview, rememberUpdatedState at :45, the rest in the two content composables). I also hand-checked the import list against body usage in the other four main-source files this PR touches (TvEmptyState.kt, TvFolderCard.kt, TvPodcastGridScaffold.kt, TvYourPodcastsScreen.kt) plus the view model — no unused imports anywhere. Caveat: I still can't run Gradle in this job (the ./gradlew call needs approval), so this is a read of the source, not an observed spotlessCheck. Your --rerun-tasks run is the authoritative one, and defeating the up-to-date check was the right call given how the first run failed.

2. The two auto-focus waits. Both are correct, and worth spelling out why the TvEmptyState one is enough, since a single withFrameNanos {} looks thin:

  • Grid (TvPodcastGridScaffold.kt:50-53) — snapshotFlow { gridState.layoutInfo.visibleItemsInfo.isNotEmpty() }.first { it }. layoutInfo is snapshot-state-backed and snapshotFlow emits its current value on collection, so this returns immediately when the grid is already laid out and otherwise resumes on the layout pass that populates it. A visible item has been composed and placed, so the requester at index 0 is attached. Exactly the right predicate.
  • TvEmptyState.kt:43-46withFrameNanos {} then request. The frame clock dispatches on Choreographer's CALLBACK_ANIMATION, which runs before CALLBACK_TRAVERSAL (measure/layout). So the effect body starts in frame N's animation phase, registers for N+1, the Button is placed during frame N's traversal, and the request at N+1 lands on an attached node. One frame genuinely suffices.

runCatching behind both waits is now a backstop rather than the mechanism, which was the point.

Rebase resolution still holds from round 7: the runCatching guard from #5680 is intact inside focusProperties { onEnter { … } } (TvPodcastGridScaffold.kt:64-71).


New this round (minor)

focusRequesters can go stale now that the effect suspends across frames

// TvPodcastGridScaffold.kt:47-53
val focusRequesters = remember(itemKeys.size) { List(itemKeys.size) { FocusRequester() } }

if (autoFocusFirstItem) {
    LaunchedEffect(Unit) {
        snapshotFlow { … }.first { it }
        runCatching { focusRequesters.firstOrNull()?.requestFocus() }
    }
}

LaunchedEffect(Unit) captures the focusRequesters instance from the composition that started it. Previously the body ran to completion immediately, so the capture couldn't age; now it parks on first { it }, and if itemKeys.size changes while parked the effect resumes holding the pre-change list — detached requesters, swallowed by runCatching, silent no-focus. Two shapes of that:

  • Scaffold first composed with itemKeys empty → captured list is empty → firstOrNull() is null, and the predicate never becomes true anyway, so the effect parks forever and auto-focus never happens even once items arrive.
  • Size changes between first composition and layout → focus lands nowhere.

Neither is reachable today: the only autoFocusFirstItem = true caller (TvFolderDetailScreen.kt:91-94) composes the scaffold only in Loaded, which is guaranteed non-empty and is a snapshot for the screen's lifetime. It becomes reachable the moment C lands and the detail list goes reactive, though — which is the same trap round 1's LaunchedEffect(focusRequesters) note was about, arriving from the other direction. val currentRequesters by rememberUpdatedState(focusRequesters) inside the effect closes it for one line.

Fix this →

Focus can be stranded coming back from an empty folder. New path, worth one device check: OK is focused, pressing it sets openedFolder = null, the focused Button is removed, and the grid it returns to has autoFocusFirstItem defaulting to false (TvYourPodcastsScreen.kt:127-130) and only restores focus from onEnter — which needs a focus event to fire. If Compose drops focus to the root instead of routing an enter into the grid, the user has to press a direction key before anything highlights. Same family as the deferred D, just flagging that the autoFocusAction path added here creates a second entry point into it.

Also still open (small)

  • Preview coverage for the new parameter. TvEmptyStatePreview (TvEmptyState.kt:79-95) uses the autoFocusAction = false default, and deleting TvFolderDetailEmpty removed the only preview of the folder-detail empty state — TvFolderDetailPreview only covers Loaded. A second preview on TvFolderDetailUiState.Empty is ~10 lines and squares the "compose previews" checklist box for the state you changed.
  • Loaded → Empty transition untested. The old unsubscribing the last podcast falls back to the empty state case went away in the rewrite; the suite now only covers Loading → Empty. Cheap to bolt onto the reactive test at TvYourPodcastsViewModelTest.kt:75 — set homeFolder = emptyList() and emit again.
  • val focusRequester = remember { FocusRequester() } (TvEmptyState.kt:41) is allocated even when autoFocusAction is false, then only wired at :71. Cosmetic.

Net: the two things I raised last round are properly fixed — the import cleanup is verified by reading, and the placement waits are the right mechanism rather than a wider catch. The rememberUpdatedState on focusRequesters is the only item I'd bother with before merge, and only because C will make it live. Everything else is optional. @geekygecko has already approved.

CI: 2 passed, 0 failed, 1 in progress (this job). Buildkite (build/lint/tests) isn't visible from here, and Gradle needs approval I don't have in this job.
· branch feat/tv-podcast-folders

@sztomek
sztomek merged commit fdc5d66 into main Aug 4, 2026
19 checks passed
@sztomek
sztomek deleted the feat/tv-podcast-folders branch August 4, 2026 08:25
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.

4 participants