[TV] Add the Up Next tab - #5679
Conversation
Generated by 🚫 Danger |
|
@claude please review this PR |
|
Claude finished @sztomek's task in 5m 26s —— View job Review: [TV] Add the Up Next tab
Overall this is clean and well-factored — extracting 1. Auto-focus will likely fight the tab bar (focus trap) — worth verifying on a device
Tab(selected = index == selectedTabIndex, onFocus = { onTabSelect(index) }, …)So merely moving the d-pad onto the "Up Next" tab composes
Notably, no other tab-level screen does this — 2.
|
2a17eb9 to
029d17f
Compare
6b526f9 to
66947c0
Compare
|
Claude finished @sztomek's task in 5m 10s —— View job Review: [TV] Add the Up Next tab (re-review after
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| LaunchedEffect(Unit) { | ||
| firstEpisodeFocusRequester.requestFocus() | ||
| } |
There was a problem hiding this comment.
Focus steal on a tab-level screen. TvTabBar.kt:76 selects a tab on focus (onFocus = { onTabSelect(index) }), so merely d-padding onto the "Up Next" tab composes this screen, which then immediately pulls focus down into the list. Same for the empty state at lines 187-189.
No other tab-level screen does this — TvHomeScreen/TvPlaylistsScreen don't request focus; only navigated-into subscreens (TvPlaylistDetailsScreen) and modals do. Please d-pad across the full tab row on a device and confirm you can traverse past Up Next. In the empty case the auto-focused Add episodes button also means a stray SELECT bounces the user to Home.
If it reproduces, either drop the auto-focus here and rely on TabRow's focusRestorer(), or gate the request on the content already owning focus.
There was a problem hiding this comment.
Fixed — dropped the auto-focus in both UpNextList and UpNextEmpty, so d-padding onto the Up Next tab (which selects on focus) no longer yanks focus into the list/button. Focus now relies on the TabRow/traversal like the other tab-level screens (TvHomeScreen, TvPlaylistsScreen).
| .stateIn( | ||
| viewModelScope, | ||
| SharingStarted.WhileSubscribed(stopTimeout = 300.milliseconds, replayExpiration = Duration.ZERO), | ||
| TvUpNextUiState.Loading, | ||
| ) |
There was a problem hiding this comment.
TvUpNextUiState.Loading is effectively dead in production, and the empty state can flash on cold start.
UpNextQueueImpl.kt:55-58 seeds the relay with State.Empty when changesObservable is first created:
val relay = BehaviorRelay.create<UpNextQueue.State>().toSerialized()
relay.accept(UpNextQueue.State.Empty)So the first emission is always Empty, before setupBlocking() has read the DB. Since that seeding is fire-and-forget from TvApplication (applicationScope.launch { upNextQueue.setupBlocking() }), a user who reaches this tab quickly sees "Nothing queued up" and then a jump to the populated list. Combined with the auto-focus in TvUpNextScreen, that's also two focus grabs in a row.
Options: expose a "queue loaded" signal from UpNextQueue and keep Loading until it fires, or drive the initial DB read from this ViewModel so Loading actually covers it.
There was a problem hiding this comment.
Deferring — the Empty-flash/Loading-dead issue is real (the relay seeds State.Empty before setupBlocking reads the DB), but the clean fix needs a "queue loaded" signal exposed from the shared UpNextQueue, which touches non-TV code. Tracking it as a follow-up rather than special-casing it in this ViewModel.
| ) | ||
|
|
||
| fun onShown() { | ||
| UpNextSyncWorker.enqueue(syncManager, context) |
There was a problem hiding this comment.
Two things about routing the refresh through UpNextSyncWorker:
-
It instantiates the whole playback graph on TV.
UpNextSyncinjectsPlaybackManagernon-lazily (UpNextSync.kt:46), so showing this tab buildsPlaybackManager→FocusManager,AudioNoisyManager,MediaSessionManager,CastManager,SleepTimer, … NoteTvSignOutManagerdeliberately takesLazy<PlaybackManager>to avoid exactly this. The good news isPlaybackManager.setup()(which callsupNextQueue.setupBlocking(),PlaybackManager.kt:302-304) is never called on TV, so there's no duplicatesendToServerBlockingsubscription — but that invariant is now load-bearing and undocumented. Worth a short comment at theTvApplicationcall site.The failure mode to check: if anything in that graph can't be constructed on a real TV device,
UpNextSyncWorker.doWork()swallows it (catch (e: Exception) … Result.failure(), log-only), so testing step 6 would silently do nothing. Please confirm on-device that you seeUpNextSyncWorker - finishedin the logs. -
Repeated enqueues chain rather than dedupe.
onShown()fires on every composition of the tab, and because the tab bar selects on focus, sweeping the d-pad across the row can trigger several.UpNextSyncWorker.enqueueusesExistingWorkPolicy.APPEND_OR_REPLACE, so those queue up sequentially instead of collapsing. Gating in the ViewModel (e.g. only sync once per ViewModel lifetime, or on lifecycleRESUMED) would be cheaper.
There was a problem hiding this comment.
Addressed point 2 — onShown() now guards on a hasSynced flag so the sync enqueues once per ViewModel lifetime instead of on every tab composition, so sweeping the d-pad across the row no longer chains APPEND_OR_REPLACE work. For point 1, added a one-line comment at the TvApplication.setupBlocking() call site documenting that it is the only such call on TV (no PlaybackManager.setup()), so a second call would double-subscribe. On-device confirmation of UpNextSyncWorker - finished is on the device-test checklist.
| @Composable | ||
| private fun episodeSummaryText(episodes: List<PodcastEpisode>): String { | ||
| val context = LocalContext.current | ||
| val countText = pluralStringResource(LR.plurals.episode_count, episodes.size, episodes.size) | ||
| val remainingMs = episodes.sumOf { episode -> | ||
| (episode.durationMs - episode.playedUpToMs).coerceAtLeast(0).toLong() | ||
| } | ||
| val timeLeftText = stringResource(LR.string.time_left, TimeHelper.getTimeDurationShortString(remainingMs, context, emptyString = "0")) | ||
| return "$countText · $timeLeftText" | ||
| } |
There was a problem hiding this comment.
The arithmetic fix reads much better now. Two follow-ups:
emptyString = "0"is unreachable.getTimeDurationShortStringfalls intosecs >= 0 && mins == 0Lfor a zero total and returns"0s", sooutputis never empty onceremainingMsis coerced non-negative — the empty queue renders "0s left", not "-". (My earlier note claiming "- left" was wrong.) You can drop the argument.- This is still a near-duplicate of
TvPlaylistDetailsScreen.kt:427, and the two have now diverged in a second way: the playlist copy special-casesepisodes.isEmpty()to omit the duration, this one doesn't. GivenTvEpisodeListItemjust moved tocomponent/, a shared helper there taking aremaining: Boolean(or two named functions) seems like the natural next extraction.
There was a problem hiding this comment.
Fixed the first point — dropped the unreachable emptyString = "0" argument. On the second (shared summary helper): agreed it is now a near-duplicate that has diverged, but extracting a shared component/ helper also touches TvPlaylistDetailsScreen and needs a decision on the remaining vs total-duration split, so tracking it as the natural follow-up now that TvEpisodeListItem lives there.
| is TvTab.UpNext -> TvUpNextScreen( | ||
| onNavigateToDiscover = { | ||
| val homeIndex = uiState.tabs.indexOfFirst { it is TvTab.Home } | ||
| if (homeIndex >= 0) { | ||
| viewModel.selectTab(homeIndex) | ||
| } | ||
| }, | ||
| ) |
There was a problem hiding this comment.
The callback is named onNavigateToDiscover but selects the Home tab — onNavigateToHome would read truer (also in TvUpNextScreen's signature). And since TvTab.entries is static, the indexOfFirst/>= 0 guard is doing nothing useful:
| is TvTab.UpNext -> TvUpNextScreen( | |
| onNavigateToDiscover = { | |
| val homeIndex = uiState.tabs.indexOfFirst { it is TvTab.Home } | |
| if (homeIndex >= 0) { | |
| viewModel.selectTab(homeIndex) | |
| } | |
| }, | |
| ) | |
| is TvTab.UpNext -> TvUpNextScreen( | |
| onNavigateToHome = { viewModel.selectTab(TvTab.entries.indexOf(TvTab.Home)) }, | |
| ) |
(Or add a selectTab(tab: TvTab) overload on the ViewModel if you'd rather keep index resolution out of the composable — note this assumes uiState.tabs is always TvTab.entries, which it is today.)
There was a problem hiding this comment.
Fixed — renamed the callback to onNavigateToHome (screen signature + call site) and simplified the resolution to TvTab.entries.indexOf(TvTab.Home), dropping the no-op indexOfFirst/>= 0 guard.
| applicationScope.launch { | ||
| upNextQueue.setupBlocking() | ||
| } |
There was a problem hiding this comment.
Thanks for moving this to Dispatchers.IO. Two smaller notes:
- The repo already has a shared
@ApplicationScope CoroutineScope(modules/services/coroutines/.../CoroutinesModule.kt:17, used byPocketCastsApplication.kt:117); injecting it instead of hand-rolling a scope would match convention. It needsimplementation(projects.modules.services.coroutines)intv/build.gradle.kts. - Worth a one-line comment that this is the only
setupBlocking()call on TV because there's noPlaybackManager.setup()here —UpNextQueueImpl.disposablesis never cleared, so a second call would double-subscribe the debouncedsendToServerBlockingpipeline. See the note onTvUpNextViewModel.onShown(): the sync worker does now construct aPlaybackManageron TV, it just never callssetup().
There was a problem hiding this comment.
Added the one-line comment at the setupBlocking() call site (the second note). Deferring the first — injecting the shared @ApplicationScope is the right convention but pulls in projects.modules.services.coroutines as a new tv dependency, so grouping it with a small follow-up rather than adding the module edge in this PR.
029d17f to
eac20ce
Compare
66947c0 to
22c96ec
Compare
|
Claude finished @sztomek's task in 1m 31s —— View job Re-review: no code changes since the last pass
The Open items, in priority order (details in the inline threads):
I didn't run Gradle in this sandbox, so |
|
Version |
eac20ce to
6700c37
Compare
22c96ec to
d1829d7
Compare
|
Claude Code is working… I'll analyze this and get back to you. |
3 participants
Description
Adds the Up Next tab to the TV app, mirroring the Apple TV implementation. The tab shows the queued episodes (the currently-playing episode is excluded, matching tvOS's
allUpNextEpisodes().dropFirst()) as a vertical list, reusing the same episode row + focus-driven…actions menu as the playlist details screen.TvEpisodeListItem(episode row + the…button that slides in on focus + the actions modal), now used by both the playlist details screen and Up Next.UpNextQueue.changesObservable. Because the TV app has noPlaybackManager, the queue relay was never seeded from the DB, soTvApplicationnow callsupNextQueue.setupBlocking()once at startup (the mobile-startup analog).UpNextQueueis a singleton, so the tab reflects the real queue and updates reactively.UpNextSyncWorker(the Android analog of tvOSrefreshManager.syncUpNext()), so changes made on other devices show up without a restart.ContentUnavailableViewaction.Scope / deliberate deviations (consistent with the rest of the TV module):
UserEpisodes (uploaded files) are filtered out — the whole TV module's episode UI isPodcastEpisode-only. tvOS shows them; rendering them here would mean generalizing the shared row. Follow-up if needed.Fixes PCDROID-578 https://linear.app/a8c/issue/PCDROID-578/up-next-tab.
Designs: Ftk3KwnfqaK4g57yCN63p0-fi-2021_5935.
Stacked on
feat/tv-playlist-episode-actions(#5676) — merge bottom-up.Testing Instructions
./gradlew :tv:installDebugProd) and sign in with an account that has episodes in Up Next.…button slides in from the right; press RIGHT then select to open the actions modal, and back to dismiss.Screenshots or Screencast
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)TvUpNextViewModelTest)modules/services/localization/src/main/res/values/strings.xml