[TV] Local folder results in search (Plus) - #5747
Conversation
|
Claude finished @sztomek's task in 5m 14s —— View job Code review
The shape of this is good — gating on 🔴 The Folders row will not lay out correctly
A Every other tile in a Related: 🟠
|
Generated by 🚫 Danger |
| TvFolderCard( | ||
| folder = folderItem.folder, | ||
| coverUrls = folderItem.podcasts.take(FOLDER_COVER_COUNT).map { PodcastImage.getMediumArtworkUrl(it.uuid) }, | ||
| onClick = { onOpenFolder(folderItem) }, | ||
| ) |
There was a problem hiding this comment.
The folder card has no width here, so this row will not lay out correctly.
Every other tile used inside TvRow carries an explicit width, because a LazyRow item nested in a LazyColumn item is measured with unbounded constraints on both axes:
TvPodcastTile→imageModifier = Modifier.width(TvPodcastTileDefaults.RowImageWidth)(TvSearchScreen.kt:711)TvSearchEpisodeCard→Modifier.width(SearchEpisodeCardWidth)TvVideoTile→.width(323.dp),TvFeaturedTile→.width(642.dp)
TvFolderCard has no intrinsic size — it derives everything from BoxWithConstraints:
BoxWithConstraints(Modifier.fillMaxWidth().aspectRatio(1f)) {
val cardWidth = maxWidth
val coverSize = cardWidth * COVER_SIZE_RATIO
…
}With unbounded constraints fillMaxWidth() is a no-op, aspectRatio(1f) finds no finite constraint to anchor to and passes the original constraints through, so maxWidth == Dp.Infinity. Every derived value (coverSize, coverSpacing, the padding(top = …), Arrangement.spacedBy(coverSpacing)) becomes Dp.Infinity and the card degenerates. In TvYourPodcastsGrid this never surfaces because the grid passes a sizing itemModifier (TvYourPodcastsScreen.kt:181).
Suggested fix — give it the same footprint as the podcast tiles in the row:
| TvFolderCard( | |
| folder = folderItem.folder, | |
| coverUrls = folderItem.podcasts.take(FOLDER_COVER_COUNT).map { PodcastImage.getMediumArtworkUrl(it.uuid) }, | |
| onClick = { onOpenFolder(folderItem) }, | |
| ) | |
| TvFolderCard( | |
| folder = folderItem.folder, | |
| coverUrls = folderItem.podcasts.take(FOLDER_COVER_COUNT).map { PodcastImage.getMediumArtworkUrl(it.uuid) }, | |
| onClick = { onOpenFolder(folderItem) }, | |
| modifier = Modifier.width(TvPodcastTileDefaults.RowImageWidth), | |
| ) |
Worth adding a @Preview with a TvSearchState.Results containing folders — the existing TvSearchScreenPreview only renders Idle, so nothing currently exercises this row.
| private suspend fun searchFolders(term: String): List<FolderItem.Folder> { | ||
| if (!userManager.getSignInState().firstOrError().await().isSignedInAsPlusOrPatron) { | ||
| return emptyList() | ||
| } | ||
| return folderManager.getAll() | ||
| .filter { it.name.contains(term, ignoreCase = true) } | ||
| .map { folder -> FolderItem.Folder(folder = folder, podcasts = podcastManager.findPodcastsInFolder(folder.uuid)) } | ||
| } |
There was a problem hiding this comment.
Subscribing to getSignInState() per search can stall the whole result set, and has side effects.
UserManager.getSignInState() is a cold chain that, on every subscription (UserManager.kt:111-145):
- fires
notificationScheduler.setupTrendingAndRecommendationsNotifications()when logged in, orcancelScheduledWorksByTag(...)when logged out — so every debounced search re-triggers WorkManager scheduling; - calls
analyticsController.refreshMetadata()on each emission; - when
settings.cachedSubscriptionis empty, falls intofetchSubscriptionForSignIn(), which is a network call retriedSUBSCRIPTION_FETCH_MAX_ATTEMPTS = 3×SUBSCRIPTION_FETCH_TIMEOUT_MS = 10_000.
Because the terminal state assignment awaits it (val folders = foldersSearch.await() on line 162), case 3 means the final Results — including all the remote episodes — can be withheld for up to ~30s on a cold subscription cache. runCatching doesn't help here; it's a hang, not a throw. That contradicts the "never delays the remote results" intent in the PR description.
The phone avoids this by subscribing once and seeding a value (SearchHandler.kt:67):
private val signInStateObservable = userManager.getSignInState().startWith(SignInState.SignedOut).toObservable()Suggest mirroring that — hoist the sign-in state into a StateFlow on the ViewModel (userManager.getSignInState().asFlow().stateIn(viewModelScope, SharingStarted.Eagerly, SignInState.SignedOut)) and read .value in searchFolders, so the gate is a cheap in-memory check.
Two smaller notes on this block:
runCatching { searchFolders(term) }(line 137) also swallowsCancellationException, turning a cancelled child into a "successful"emptyList(). Harmless today since the parent is being cancelled anyway, but it's inconsistent with the explicitcatch (exception: CancellationException) { throw exception }pattern used everywhere else in this file. Acatch (e: Exception)after re-throwing cancellation would match the house style.- The cover artwork uses
podcastManager.findPodcastsInFolder(raw DB order) while the detail screen usesfolderManager.findFolderPodcastsSorted. This matches the phone, so it's fine for parity, but the four covers on a card can differ from what the same folder shows in Your Podcasts.
| whenever(listRepository.getSearchDiscoverFeed()).thenReturn(discover()) | ||
| whenever { improvedSearchManager.combinedSearch(any()) }.thenReturn(emptyList()) | ||
| whenever(userManager.getSignInState()).thenReturn(Flowable.just(plusSignInState())) | ||
| whenever { folderManager.getAll() }.thenReturn(listOf(folderEntity("Sugar"))) | ||
| whenever { podcastManager.findPodcastsInFolder(any()) }.thenReturn(emptyList()) | ||
|
|
||
| val viewModel = createViewModel() | ||
| viewModel.onQueryChange("sugar") | ||
| advanceUntilIdle() | ||
|
|
There was a problem hiding this comment.
The headline behaviour of commit 49df96f ("degrade gracefully on failure") isn't covered. Worth one more case asserting that a folder-lookup failure doesn't take the search down:
@Test
fun `a folder lookup failure does not fail the search`() = runTest {
whenever(listRepository.getSearchDiscoverFeed()).thenReturn(discover())
whenever { improvedSearchManager.combinedSearch(any()) }.thenReturn(listOf(podcastItem("podcast-1")))
whenever(userManager.getSignInState()).thenReturn(Flowable.just(plusSignInState()))
whenever { folderManager.getAll() }.thenThrow(RuntimeException("db"))
val viewModel = createViewModel()
viewModel.onQueryChange("sugar")
advanceUntilIdle()
val state = viewModel.searchState.value as TvSearchState.Results
assertEquals(listOf("podcast-1"), state.podcasts.map { it.uuid })
assertTrue(state.folders.isEmpty())
}
Description
Adds local folder results to Android TV Search. Stacked on the TV search parity PR (#5746).
Behaviour
TvFolderDetailScreen(reusing the exact overlay/nav pattern from the Your Podcasts tab, including focus-restore and podcast-opened-from-folder returning to the folder).Implementation
TvSearchViewModelinjectsFolderManager+UserManager.searchFolders()gates onSignInState.isSignedInAsPlusOrPatron, filtersfolderManager.getAll()by name, attaches each folder's podcasts. It runs in arunCatching { … }asyncconcurrently with the network search, so a (rare) local-DB failure degrades to "no folders" rather than failing the whole search, and it never delays the remote results.foldersadded toTvSearchState.Results(+ the NoResults emptiness check).folderPodcasts(uuid)feeds the detail screen.TvFolderCardandTvFolderDetailScreencomponents.Reuses existing infra throughout — no shared-module changes, tv-only.
Notes
SearchOpenedFolder/its saver duplicate the private equivalents inTvYourPodcastsScreen; deferred as a small shared-extraction cleanup (would touch the unrelated Your Podcasts screen).Testing Instructions
./gradlew :tv:installDebug; open Search, type the folder's name → a Folders row appears in Top Results; open a folder → its podcasts; open a podcast → back returns to the folder../gradlew :tv:testDebugUnitTest— covers Plus surfaces folders, non-Plus skips the lookup, folder-alone counts as a result.Screenshots or Screencast
Pending device verification (needs a Plus account) — will attach.
Checklist
./gradlew spotlessApply)folders