Skip to content

[TV] Local folder results in search - #5747

Merged
sztomek merged 8 commits into
mainfrom
feat/tv-search-folders
Aug 27, 2026
Merged

[TV] Local folder results in search #5747
sztomek merged 8 commits into
mainfrom
feat/tv-search-folders

Conversation

@sztomek

@sztomek sztomek commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Description

Adds local folder results to Android TV Search. Stacked on the TV search parity PR (#5746).

Behaviour

  • When a signed-in user searches, their local folders whose name matches the term (case-insensitive) are surfaced, each with its podcasts' artwork.
  • Signed-out users get nothing here — the folder lookup is skipped entirely (no DB hit).
  • Folders appear as a Folders carousel in Top Results, ordered Featured → Episodes → Folders → Podcasts; tapping one opens the folder in TvFolderDetailScreen (reusing the exact overlay/nav pattern from the Your Podcasts tab, including focus-restore and podcast-opened-from-folder returning to the folder).
  • When a search yields at least one folder, a dedicated Folders filter pill appears after Episodes and shows a folder-only grid. The pill is hidden when there are no folder results; if the user is on the Folders filter and a subsequent search has no folders, the view transparently falls back to Top Results.

Implementation

  • TvSearchViewModel injects FolderManager. searchFolders() gates on syncManager.isLoggedIn(), filters folderManager.getAll() by name, attaches each folder's podcasts. It runs in a runCatching { … } async concurrently 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.
  • folders added to TvSearchState.Results (+ the NoResults emptiness check). folderPodcasts(uuid) feeds the detail screen.
  • New TvSearchFilter.Folders entry (after Episodes). The visible pill set is derived from the current results — Folders is only included when the results contain folders — and an effectiveFilter keeps the selected pill and the rendered content in sync without mutating ViewModel state when folders come and go. TvSearchFilters now takes the filter list to render.
  • UI reuses the existing TvFolderCard, TvFolderDetailScreen, and TvPodcastGridScaffold components (the Folders filter grid mirrors the Podcasts filter layout).

Folders are inherently a paid feature (a free user can't create them, and they're wiped on downgrade), so the gate only needs to check that the user is signed in. This matches how the rest of the TV module treats folders — the Your Podcasts tab renders them straight from FolderManager with no subscription check. An earlier revision gated on SignInState.isSignedInAsPlusOrPatron, but the subscription tier is not reliably hydrated on TV (it's the only place in the module that read it), which silently suppressed every folder even for Plus users; gating on isLoggedIn() — the same guard this ViewModel already uses elsewhere — fixes that.

Reuses existing infra throughout — the only shared-module change is the new search_filters_folders string alias; otherwise tv-only.

UPDATE
got design approval: p1787763569156349/1787663272.838409-slack-C0ATWH7BNH3

Testing Instructions

  1. Sign in with at least one folder whose name matches a query.
  2. ./gradlew :tv:installDebug; open Search, type the folder's name → a Folders row appears in Top Results, and a Folders filter pill appears after Episodes; select it → a folder-only grid; open a folder → its podcasts; open a podcast → back returns to the folder.
  3. Refine the query so no folder matches → the Folders pill disappears and the view falls back to Top Results.
  4. As a signed-out user, neither the Folders row nor the Folders pill ever appears.
  5. ./gradlew :tv:testDebugUnitTest — covers signed-in surfaces folders, signed-out skips the lookup, folder-alone counts as a result.

Screenshots or Screencast

Screen_recording_20260824_174641.mp4

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md — N/A (TV, pre-release)
  • Ensure the linter passes (./gradlew spotlessApply)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in localization — reuses existing folders via a search_filters_folders alias
  • 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 — N/A (no analytics change)

@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

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


Code review

  • Gather context (diff vs feat/tv-search-parity)
  • Review TvSearchViewModel.kt
  • Review TvSearchScreen.kt
  • Review tests
  • Post review

The shape of this is good — gating on isSignedInAsPlusOrPatron, running the folder lookup concurrently with the network search, and reusing TvFolderCard / TvFolderDetailScreen all mirror the existing patterns closely. The overlay wiring (folder overlay inactive while a podcast is open, folderRestoreTrigger in the podcast overlay's onHide) matches TvYourPodcastsScreen exactly, including the podcast-opened-from-folder return path. Two things I'd want fixed before merge, both in the "device verification pending" blind spot.


🔴 The Folders row will not lay out correctly

TvSearchScreen.kt:604TvFolderCard is placed in a TvRow with no width modifier.

A LazyRow item nested inside a LazyColumn item is measured with unbounded constraints on both axes. TvFolderCard has no intrinsic size: it derives coverSize, coverSpacing, padding(top = …) and Arrangement.spacedBy(…) from BoxWithConstraints.maxWidth, and Modifier.fillMaxWidth().aspectRatio(1f) can't anchor to anything under unbounded constraints, so maxWidth resolves to Dp.Infinity and every derived dimension degenerates. It works in TvYourPodcastsGrid only because the grid passes a sizing itemModifier.

Every other tile in a TvRow sets its own width — TvPodcastTile (RowImageWidth), TvSearchEpisodeCard (SearchEpisodeCardWidth), TvVideoTile (323.dp), TvFeaturedTile (642.dp). Details and a suggested fix are in the inline comment.

Related: TvSearchScreenPreview only renders TvSearchState.Idle, so no preview exercises the new row — which is why this wasn't caught. The checklist item about preview coverage isn't really satisfied yet.

Fix this →

🟠 getSignInState() per search can stall the results

TvSearchViewModel.kt:182userManager.getSignInState().firstOrError().await() subscribes to a cold chain on every debounced search. That chain (UserManager.kt:111-145):

  • re-triggers notificationScheduler.setupTrendingAndRecommendationsNotifications() / cancelScheduledWorksByTag(...) per subscription,
  • calls analyticsController.refreshMetadata() per emission,
  • falls into fetchSubscriptionForSignIn() — a network call, 3 attempts × 10s timeout — when settings.cachedSubscription is empty.

Since line 162 awaits foldersSearch before emitting the terminal state, the third case can withhold the entire result set (episodes included) for up to ~30s. runCatching doesn't cover it — it's a hang, not a throw. The phone deliberately avoids this by subscribing once with a seed value (SearchHandler.kt:67: getSignInState().startWith(SignInState.SignedOut)); hoisting it into a StateFlow on the ViewModel and reading .value would make the gate free.

Fix this →

Smaller notes

  • runCatching swallows cancellation (TvSearchViewModel.kt:137) — a cancelled child resolves to a "successful" emptyList(). Harmless in practice, but inconsistent with the explicit catch (CancellationException) { throw } pattern used everywhere else in this file.
  • Missing test for the graceful-degradation path — commit 49df96f's whole point (a folder lookup failure shouldn't fail the search) isn't asserted. Suggested test in the inline comment.
  • Remote failure discards matched folders — if fullSearch.await().getOrThrow() throws, the state becomes Error even for a Plus user whose local folders matched. Since folder search is purely local, those results could still be shown. Arguably out of scope, but it's a behaviour the folder feature newly makes visible.
  • Cover artwork ordering — the card uses podcastManager.findPodcastsInFolder (raw DB order) while the detail screen uses findFolderPodcastsSorted. Matches the phone, so fine for parity; just means a folder's four covers can differ between Search and Your Podcasts.
  • DuplicationSearchOpenedFolder/its saver and FOLDER_COVER_COUNT now exist twice (here and TvYourPodcastsScreen.kt:100-105, :187). Already acknowledged in the description; agreed it's a fine follow-up. Trivial nit: the saver uses ?: emptyList() where the original uses .orEmpty().

Nothing security-relevant here — the Plus gate is a UI-surfacing decision over data already on-device, and there's no new network surface.

I did not build or run the tests (no Gradle run in this environment), so the layout finding is from reading the constraint flow rather than from a screenshot — but it lines up with the note in the PR that device verification is still pending.
• branch feat/tv-search-folders

@sztomek
sztomek force-pushed the feat/tv-search-parity branch from 489431e to a596632 Compare August 24, 2026 10:27
@sztomek
sztomek force-pushed the feat/tv-search-folders branch from 49df96f to 71d0c7e Compare August 24, 2026 10:35
@sztomek
sztomek force-pushed the feat/tv-search-parity branch from a596632 to 21d3ea9 Compare August 24, 2026 12:51
@sztomek
sztomek force-pushed the feat/tv-search-folders branch from 71d0c7e to 63a7489 Compare August 24, 2026 12:53
@sztomek sztomek changed the title [TV] Local folder results in search (Plus) [TV] Local folder results in search Aug 24, 2026
@sztomek sztomek added this to the 8.20 milestone Aug 24, 2026
@sztomek
sztomek marked this pull request as ready for review August 24, 2026 15:50
@sztomek
sztomek requested a review from a team as a code owner August 24, 2026 15:50
@sztomek
sztomek requested review from geekygecko and removed request for a team August 24, 2026 15:50
@claude

claude Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

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


Code review

  • Gather context (diff vs feat/tv-search-parity)
  • Review TvSearchViewModel.kt
  • Review TvSearchScreen.kt / TvSearchFilters.kt
  • Review tests
  • Post review

Everything from the last round is genuinely fixed, and the fixes are the right ones rather than the minimal ones:

  • TvFolderCard now carries Modifier.width(TvPodcastTileDefaults.RowImageWidth) in the row, matching every other tile in a TvRow, and TvSearchResultsPreview actually renders a Results state containing a folder — so the row has preview coverage now.
  • The gate moved to syncManager.isLoggedIn(), which drops the getSignInState() cold-chain subscription entirely. That's a better outcome than the StateFlow hoist I suggested: no per-search resubscription, no refreshMetadata()/WorkManager side effects, no fetchSubscriptionForSignIn() stall, and it's the same guard the ViewModel already uses at line 102. The reasoning in the description about tier not being hydrated on TV holds up.
  • CancellationException is now rethrown explicitly before the catch-all, matching the house style used throughout the file.
  • The graceful-degradation test landed.

The matching logic is a faithful port of the phone (name.contains(query, ignoreCase = true), deleted folders excluded by the DAO), and the async structure is sound — both async children swallow their own failures, so neither can cancel the parent through the async-failure path.

Two things left, neither a correctness bug. Details inline.


🟠 The Folders filter re-applies itself, including mid-search

TvSearchScreen.kt:292-294effectiveFilter corrects what's rendered but never writes back, so the ViewModel keeps holding Folders after the pill disappears. The fallback is cosmetic: as soon as folders return, the content jumps to the Folders grid unprompted.

This is visible within one search, not just across searches, because the partial emission (TvSearchViewModel.kt:160) carries no folders. For a user on the Folders filter, refining the query renders Searching → Top Results (partial, pill gone) → Folders grid (full, pill back). The content area swaps twice and focus travels with it — rough on a D-pad if they'd already started navigating the row that gets replaced.

Fix is two small changes: don't derive hasFolders from a partial state (isPartial means "not resolved yet", not "none"), and reset _filter to TopResults in the ViewModel when the terminal result has no folders.

Worth noting Folders is the only pill that can vanish — Podcasts and Episodes stay put and render a no-results message instead — so this is a transition nothing else in the filter row has to survive.

Fix this →

🟡 Folder ordering and cover artwork

TvSearchViewModel.kt:193-195 — two related points:

  • Results are in unspecified DB order. getAll() is SELECT * FROM folders WHERE deleted = 0 with no ORDER BY. The phone avoids this by sorting the merged list (SearchHandler.kt:109); TV never sorts. Matters most for the Folders grid, which isn't capped at SEARCH_ROW_LIMIT.
  • Covers use findPodcastsInFolder (unsorted) while the detail screen and Your Podcasts both use findFolderPodcastsSorted. The parity argument is weaker here than on phone — on TV the same folder's four covers can differ between Search and Your Podcasts — and it's a one-identifier change that also reduces this to a single source. Suggestion inline.

Also noted there: one findPodcastsInFolder per match, sequential and uncapped, per debounced keystroke, when only four covers are ever used. Local and debounced, so informational.

Smaller notes

  • filters list identity (TvSearchScreen.kt:293) — TvSearchFilter.entries.filter { … } allocates each recomposition, and List is an unstable parameter type, so TvSearchFilters and its TabRow are no longer skippable and recompose on every keystroke. remember(hasFolders) { … } fixes both.
  • No folder analytics from searchonFolderImpression = {} at line 176, whereas Your Podcasts tracks trackFolderShown/trackFolderTapped. Consistent with the "no analytics change" checklist item, just flagging that opening a folder from Search is invisible to Tracks while the identical action from Your Podcasts isn't.
  • DuplicationSearchOpenedFolder, its saver, and FOLDER_COVER_COUNT are now near-copies of TvYourPodcastsScreen.kt:128-137. Already acknowledged as a follow-up; agreed.
  • Remote failure still discards matched foldersgetOrThrow() throwing puts the state in Error even when local folders matched. Unchanged from the last round and still reasonable to leave out of scope.

Nothing security-relevant: the gate is a UI-surfacing decision over on-device data, and there's no new network surface. Folder names go into a contains comparison, not a query string, so there's no injection path.

I couldn't run Gradle in this environment, so spotlessCheck and :tv:testDebugUnitTest are unverified from my side — I did confirm the new test file's Date, never, and verifyBlocking imports were already present, so the tests should at least compile.
• branch feat/tv-search-folders

@sztomek sztomek added the [Type] Feature Adding a new feature. label Aug 24, 2026
Comment thread tv/src/main/java/au/com/shiftyjelly/pocketcasts/search/TvSearchScreen.kt Outdated
Comment thread tv/src/main/java/au/com/shiftyjelly/pocketcasts/search/TvSearchViewModel.kt Outdated

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

I don't see search by folder on mobile or Apple TV search but it seems like a good idea. Maybe we should ask the designers about the extra tab.

Image

Thanks again for including a video in your PR description.

Base automatically changed from feat/tv-search-parity to main August 25, 2026 09:31
@sztomek
sztomek force-pushed the feat/tv-search-folders branch from f474792 to a527653 Compare August 25, 2026 12:57
@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

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


Code review

  • Gather context (diff since the last review — a527653)
  • Review TvSearchViewModel.kt
  • Review TvSearchScreen.kt / TvSearchFilters.kt
  • Review tests
  • Post review

a527653 picks up both items from the last round. The ViewModel side is right:

  • Filter reset is now real. _filter drops back to TopResults when the terminal result has no folders (TvSearchViewModel.kt:174-176), placed before the NoResults/Results split so it covers both. Ordering is fine — _filter settles before _searchState is assigned, so no recomposition sees a filter that isn't in the pill list.
  • Sorting matches the phone. sortedBy { PodcastsSortType.cleanStringForSort(it.name) } is the same key as SearchHandler.kt:109, and cleanStringForSortInternal lowercases + strips a leading "the" + removes accents, so it's case-insensitive — no "Banana" < "apple" surprise. Covered by a test.
  • Covers come from findFolderPodcastsSorted now, so the Search row, the Folders grid, the detail screen and Your Podcasts all agree on artwork order, and it's one identifier instead of two managers. Stubs in the existing tests were updated with it.
  • remember(hasFolders) restores TvSearchFilters' skippability.

The hasFolders half is where I'd push back: || it.isPartial trades the old flip-flop for a phantom pill, and it makes a second, pre-existing gap reachable. Details inline.


🟠 The Folders pill now flashes in during almost every search

TvSearchScreen.kt:292isPartial forces hasFolders true, but the partial emission fires whenever earlyPodcasts is non-empty, which is most searches (autocomplete + local subscriptions). So the row goes 3 pills (Searching) → 4 (partial) → 3 (full, no folders), and since the pills are centred between two weight(1f) dividers, all of them shift horizontally twice per query. A signed-out user — who can never get a folder result — sees this too.

It's interactive in that window: Tab selects on focus (TvSearchFilters.kt:82), so reaching the phantom pill sets _filter = Folders and the new reset then bounces the user back to TopResults.

Root cause is deriving pill visibility from a state that has no answer yet. The ViewModel already has the right hook — a hasFolderResults: StateFlow<Boolean> set next to the existing _filter reset makes the pill change only on settled states, kills both the old flip-flop and this flash, and closes the Error-path case where _filter is left on Folders (a failed search followed by a folder-matching one still jumps to the Folders grid unasked).

Fix this →

🟠 Folders filter renders an empty grid where the other filters show a spinner

TvSearchScreen.kt:527Podcasts and Episodes both branch on results.isPartial to distinguish "not resolved yet" from "genuinely none"; Folders goes straight into TvPodcastGridScaffold. Keeping the pill alive through the partial state makes that path reachable: a user on the Folders filter refining a query now gets TvSearchLoadingempty grid → populated grid, and the empty grid has nothing focusable, so focus leaves the content area and has to be re-acquired. Same if (isEmpty) { if (isPartial) Loading else Message } shape as its neighbours fixes it.

Smaller notes

  • findFolderPodcastsSorted costs an extra query per folder — it starts with findByUuid(folderUuid) (FolderManagerImpl.kt:200) to read podcastsSortType, so the N+1 from the last round is now ~2N — and the Folder it re-fetches is already in hand from getAll(). Local, on IO, behind the debounce, and correctness is worth it; noting it because the fan-out is still uncapped and only four covers are ever used.
  • No folder analytics from searchonFolderImpression = {}, while Your Podcasts tracks trackFolderShown/trackFolderTapped. Consistent with the checklist; flagging that the same action is visible to Tracks from one surface and not the other.
  • DuplicationSearchOpenedFolder, its saver, and FOLDER_COVER_COUNT are still near-copies of the Your Podcasts versions. Acknowledged as a follow-up; agreed.
  • Remote failure discards matched foldersgetOrThrow() throwing yields Error even when local folders matched. Unchanged, still fine to leave out of scope.

Nothing security-relevant: the gate is a UI-surfacing decision over on-device data, folder names go into a contains comparison rather than a query string, and there's no new network surface.

I couldn't run Gradle here, so spotlessCheck and :tv:testDebugUnitTest are unverified from my side. PodcastsSortType is already a dependency of the tv module via other call sites, and the two changed stubs use an existing FolderManager method, so the new test should compile.
• branch feat/tv-search-folders

Comment thread tv/src/main/java/au/com/shiftyjelly/pocketcasts/search/TvSearchScreen.kt Outdated
Comment thread tv/src/main/java/au/com/shiftyjelly/pocketcasts/search/TvSearchScreen.kt Outdated
@sztomek

sztomek commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

@geekygecko good point, asked the designers here: p1787663272838409-slack-C0ATWH7BNH3

@sztomek
sztomek force-pushed the feat/tv-search-folders branch from a527653 to 023bbdf Compare August 26, 2026 14:25
@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

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


Code review

  • Gather context (diff since the last review — 023bbdf)
  • Review TvSearchViewModel.kt
  • Review TvSearchScreen.kt / TvSearchFilters.kt
  • Review tests
  • Post review

023bbdf closes both items from the last round, and it took the better of the two routes for each:

  • hasFolderResults is now ViewModel state. _hasFolderResults only moves on settled outcomes — terminal success, Error, and empty term — so the phantom pill during the partial emission is gone, and so is the old flip-flop. updateFolderResults(…) folding the flag and the _filter reset into one call means the two can't drift: there's no path that clears the flag without also releasing the Folders selection. It's called on all three exits (onQueryChange empty term, terminal, catch), which also closes the Error-path gap I flagged — a failed search no longer leaves _filter parked on Folders.
  • The Folders branch now has the isPartial guard. if (isEmpty) { if (isPartial) Loading else Message } is structurally identical to its Podcasts and Episodes neighbours, so the three branches read as one shape and the empty-grid/focus-drop window is closed.
  • Ordering within updateFolderResults is safe. _hasFolderResults and _filter are written back-to-back with no suspension between them, and both collectors resume on dispatch, so no recomposition can observe hasFolderResults = false while filter is still Folders. effectiveFilter remains as a belt-and-braces guard against TabRow's selectedTabIndex = -1.
  • Tests cover all three: flag across terminal/empty-term, filter reset on a folderless terminal result, and filter reset on the Error path. The Error test is meaningful rather than tautological — it only passes because of the reset in the catch.

I checked the rest of the async block again while I was in there: fullSearch's runCatching (which does swallow cancellation) is pre-existing on main, not something this PR introduced, and a leftover foldersSearch after getOrThrow() throws just completes and discards its result inside a job that's about to be cancelled anyway. No leak.

No blocking findings this round. What's left is small.


Smaller notes

  • The pill is now stale-visible for one search instead of flashing in. The flag holds its last settled value through Searching and the partial emission, so a user whose previous query matched a folder sees 4 pills for the duration of the next search, then 3. That's one centre-aligned shift instead of the two from || it.isPartial, and only for users who just had a folder hit — a strictly better trade, and "keep the last known answer until resolved" is the right default. The residual quirk: during that window the stale pill is still focus-selectable (onFocus selects), so a user who reaches it gets bounced back to TopResults when the terminal result lands. Removing it entirely would need a tri-state (unknown/yes/no), which isn't worth the complexity for this.
  • Clearing the query now resets the filter, but only from Folders (TvSearchViewModel.kt:132) — Podcasts and Episodes survive a query clear and re-apply to the next search, Folders doesn't. Falls out of reusing updateFolderResults for the Idle transition. Defensible (the pill can't be guaranteed to exist next time), just noting it's a new asymmetry rather than an intentional decision.
  • No preview covers the rewritten Folders grid branch. TvSearchResultsPreview passes filter = TopResults, hasFolderResults = true, which exercises the Top Results row but not the grid the diff just restructured. A second preview with filter = TvSearchFilter.Folders would cover it — cheap, and the same gap that let the original width bug through.
  • updateFolderResults reads as a setter but also mutates the filter. The coupling is the point, so this is naming only — something like onFolderResultsChanged would signal the side effect.
  • findFolderPodcastsSorted per matching folder, uncapped (TvSearchViewModel.kt:211) — ~2 queries per match (it re-reads the Folder you already hold, to get podcastsSortType), and only 4 covers are ever rendered in either surface. Local, on IO, behind the debounce — unchanged from last round, still informational.
  • No folder analytics from searchonFolderImpression = {}, while Your Podcasts tracks trackFolderShown/trackFolderTapped. Consistent with the checklist; flagging that the same action is visible to Tracks from one surface and not the other.
  • DuplicationSearchOpenedFolder, its saver, and FOLDER_COVER_COUNT are still near-copies of the Your Podcasts versions. Acknowledged as a follow-up; agreed.
  • Remote failure discards matched foldersgetOrThrow() throwing yields Error even when local folders matched. Unchanged, still fine to leave out of scope.

Nothing security-relevant: the gate is a UI-surfacing decision over on-device data, folder names go into a contains comparison rather than a query string, and there's no new network surface.

Gradle is blocked in this environment, so spotlessCheck and :tv:testDebugUnitTest are unverified from my side. I did confirm the new tests' helpers (folderEntity, onFilterSelected) and the assertFalse import are all present, so they should compile.
• branch feat/tv-search-folders

@sztomek
sztomek merged commit c015d5e into main Aug 27, 2026
20 checks passed
@sztomek
sztomek deleted the feat/tv-search-folders branch August 27, 2026 07:10
@claude claude Bot mentioned this pull request Aug 27, 2026
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants