Skip to content

[TV] Search analytics - #5769

Merged
sztomek merged 6 commits into
mainfrom
feat/tv-analytics-search
Aug 27, 2026
Merged

[TV] Search analytics#5769
sztomek merged 6 commits into
mainfrom
feat/tv-analytics-search

Conversation

@sztomek

@sztomek sztomek commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Description

Brings the Android TV Search screen to analytics parity with the Apple TV app (Pocket Casts TV App/UI/Search/* + Analytics/SearchAnalytics.swift). The TV Search screen previously fired zero analytics.

Apple TV is the source of truth for which events fire, their properties, and their triggers. Each event maps to the existing generated EventHorizon Kotlin class (no schema change is needed — every event already includes the android platform). source is SourceViewType.Search for the search_* events and the string "search" for the reused discover events.

This also extracts a reusable TvDiscoverFeedAnalytics tracker (parameterised by source) out of TvHomeViewModel, so the search idle body — which reuses the shared discover grid, exactly like Apple TV's DiscoverAllView(source: .search) — fires the same discover family with source="search". The Home refactor is behaviour-preserving (source="home", local-row gating intact).

Core search events (source = search)

Event Properties Trigger
search_shown source Search screen first shown (CallOnce)
search_performed source Debounced query kicks off a network search
search_filter_tapped source, filter The selected filter changes — by focus move or click (top_resultsallResults, podcasts, episodes; the TV-only folders pill maps to unknown — the schema enum has no folders value)
search_predictive_term_tapped source, term An autocomplete suggestion is tapped
search_history_item_tapped source, type=search_term A recent-search item is tapped
search_empty_results source, term Search returns no podcasts/episodes/folders
search_failed source, term Search errors
search_result_tapped source, uuid, result_type A podcast (podcast_local_result/podcast_remote_result) or episode result is opened/played (incl. Go to podcast from an episode result’s actions modal)

Discover idle-body family (source = search, via the shared tracker)

Event Properties Trigger
discover_list_impression list_id, source Each discover row shown in the idle body
discover_list_podcast_tapped list_id, podcast_uuid, source A discover podcast (or an episode's podcast) is opened
discover_featured_podcast_tapped podcast_uuid A featured discover podcast is opened
discover_ad_category_tapped name, region, id, podcast_id A sponsored discover podcast is opened
discover_categories_pill_tapped name, region, index, visits, sponsored, source A category pill is tapped (browse-categories row; the discover categories-row wiring is forward-looking — see below)

Notable parity decisions / divergences

  • search_failed carries term, not error_code — the schema contract for search_failed is source + term (checked against trunk); there is no error_code property on this event for any platform. Apple TV happens to send an undeclared error_code (its Tracks pipeline ships arbitrary property dicts) and omits the declared term, so it's the off-contract one. Android's source + term is fully compliant and matches the phone app. (Extending the contract with error_code: text was considered and declined — Android's failure path catches a generic Throwable with no uniform numeric code.)
  • search_filter_tapped — top results — Apple TV emits filter="top_results"; the Android typed SearchResultFilterType enum has no such value, so the top-results scope emits its nearest equivalent allResults (the same mapping the phone app uses for its "All" filter).
  • search_filter_tapped — fires on filter change, like tvOS — tvOS tracks this via onChange(of: model.scope), so merely moving between the chips fires the event (confirmed on device by @geekygecko). Android TV mirrors that: the pills are focus-to-select, and the event fires whenever the selected filter actually changes (focus move or click), deduped so re-selecting the current filter doesn't re-fire.
  • The discover categories row cannot fire on Search today — the search feed is built with includeHomeSections = false, and TvDiscoverFeedLoader drops ListType.Categories rows in that mode, so only the browse-categories row is a live source of discover_categories_pill_tapped on Search. The categories-row wiring is kept so it becomes correct the moment home sections are ever enabled here — don't expect two feeds of that event in the data.
  • Search discover episode-play is deferred — playing an episode from the search discover feed is a pre-existing functional TODO on Android, so discover_list_episode_tapped / discover_list_episode_play are not wired from the search idle yet (the shared tracker supports them; Home already fires them).

Testing Instructions

  1. Build & install the TV app: ./gradlew :tv:installDebug (or run the tv config).
  2. On the Search tab, watch logcat for LoggingAnalyticsListener events (debug builds log every EventHorizon event).
  3. Verify:
    • Opening Search logs search_shown; the idle body logs discover_list_impression per row.
    • Typing a query (after the ~300ms debounce) logs search_performed.
    • A query with no results logs search_empty_results; a network failure logs search_failed.
    • Tapping an autocomplete suggestion logs search_predictive_term_tapped; tapping a recent search logs search_history_item_tapped.
    • Moving focus between the filter pills logs search_filter_tapped per filter change (matching tvOS); staying on the same pill does not re-log it.
    • Opening a subscribed podcast result logs search_result_tapped with podcast_local_result; a non-subscribed one with podcast_remote_result; playing an episode result logs episode.
    • In the idle body: opening a discover podcast logs discover_list_podcast_tapped; tapping a category pill logs discover_categories_pill_tapped (all with source=search).

Screenshots or Screencast

logcat_search_analytics

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md — n/a, analytics only
  • Ensure the linter passes (./gradlew spotlessApply)
  • I have considered whether it makes sense to add tests for my changes — added VM unit tests for every new event
  • All strings that need to be localized are in modules/services/localizationn/a, no new strings
  • Any jetpack compose components I added or changed are covered by compose previews — existing previews updated
  • I have updated (or requested that someone edit) the Event Horizon schema — n/a, all events already include the android platform

@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @sztomek's task in 6m 31s —— View job


Code review

  • Gather context (read diff + surrounding code)
  • Review new TvDiscoverFeedAnalytics extraction & Home refactor
  • Review search screen / view model / filters analytics
  • Review tests
  • Post findings

Overall this is well-executed. The TvDiscoverFeedAnalytics extraction is genuinely behaviour-preserving (the Home VM now just delegates, and TvHomeViewModelTest still exercises every branch through those delegates, so the refactor is covered), the parity table in the description matches what the code actually emits, and the divergences are documented rather than hidden. A few things worth addressing before merge — 5 inline comments posted, summarised here.

Worth addressing

1. search_filter_tapped is effectively unfireableinline

This is my main finding. The pills use focus-to-select, and on a D-pad you must focus an element before you can click it — so onFocusonFilterSelected has already applied the filter and swapped the results by the time onClick can fire. onFilterClicked therefore only fires on a redundant select press on the pill the user is already on, which most users won't make. The event won't be "explicit taps only", it'll be "the small biased subset of users who press select out of habit".

The traversal concern that motivated the split is real, but a settle-debounce on focus solves it without discarding the signal: cancel a pending track job on each focus change, fire after ~500ms if the filter is unchanged and differs from the last tracked value, and have onFilterClicked cancel-and-fire immediately through the same dedup. Sketch in the inline comment. If you'd rather keep it as-is, that's defensible — but the PR should say the event is expected to be sparse so nobody reads it as real filter usage later.

2. Navigation callbacks silently became optionalinline

onOpenPodcast / onOpenCategory were required params; they've been replaced by onPodcastResultClick / onDiscoverPodcastClick / onDiscoverEpisodePodcastClick / onDiscoverCategoryClick, all with no-op defaults. Those four now carry navigation as well as tracking (TvSearchScreen.kt:488, :677 open the podcast through onPodcastResultClick), so a call site that forgets one gets dead tiles, not just a missing event — with no compiler help. Suggest making those four required and leaving only onDiscoverListImpression defaulted.

3. Search-side discover coverage gapinline

The whole point of the extraction is source="search" on the discover family, but the only test covering it goes through the category pill. trackDiscoverListShown / trackDiscoverPodcastTapped / trackDiscoverEpisodePodcastTapped have no search-side assertions — TvHomeViewModelTest already has the mirror images, so it's a cheap copy.

Smaller points

4. localRowIds = emptySet() defaultinline. Correct today (search calls buildRows(discover, isLoggedIn) with includeHomeSections = false, so no local rows and no Categories rows reach the search feed), but getting it wrong fails silently by emitting synthetic ids like keep_listening as list_id. Dropping the default forces the call site to decide.

5. Episode search_result_tapped placementinline. Baked into playEpisode() while the podcast half is an explicit trackPodcastResultTapped call from the screen. Splitting it out keeps the pair symmetric and avoids a future playEpisode caller emitting the event from a non-result context.

6. search_list_shown is absent. The phone app fires SearchListShownEvent when results are shown (SearchViewModel.reportResultsShown), and it isn't in the parity table. If Apple TV's SearchAnalytics.swift has no equivalent then this is correct as-is — just flagging it since the TV/phone datasets will differ on that one event.

7. Timing semantics differ from the phone. Phone fires search_empty_results / search_failed from the UI when those states are displayed; TV fires them from the VM when the state is produced. TV's version is cleaner and I wouldn't change it, but it means the two platforms count these slightly differently (e.g. TV counts a failure the user navigated away from before seeing).

8. History vs. suggestion asymmetry. selectSuggestion saves the term to history, selectHistoryItem doesn't — so tapping a recent search doesn't bump its recency. Pre-existing (the old wiring was viewModel::onQueryChange), not a regression from this PR, but now that the two paths are named methods sitting side by side the asymmetry is more visible.

Verified as correct

  • SearchResultFilterType.AllResults for top results matches what the phone app already does for its "All" filter (SearchViewModel.kt:376), so this isn't just an Apple-TV divergence — it's the established Android mapping.
  • search_performed can't fire on a cancelled query: it sits after the delay(SEARCH_DEBOUNCE_MS) in a job that onQueryChange cancels, and the blank-term path returns before launching.
  • No spurious search_failed from cancellation — runCatching does swallow CancellationException, but getOrThrow() rethrows it into the catch (CancellationException) branch above the SearchFailedEvent call.
  • isFollowed correctly drives local-vs-remote: Podcast.toSearchItem() hardcodes isFollowed = true and the localUuids re-stamp at lines 169/178 covers remote hits that are also subscribed.
  • Line lengths: .editorconfig sets max_line_length=off, so the long delegation one-liners are fine despite the 120-char guidance in AGENTS.md.

I did not build the module or run the tests (no Gradle dependency cache available in this environment), so the above is static analysis only. CI on this run shows the Gradle wrapper validation passing; the unit-test and spotlessCheck jobs hadn't reported yet when I looked.
· branch feat/tv-analytics-search

@dangermattic

dangermattic commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator
2 Warnings
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ Class TvDiscoverFeedAnalytics is missing tests, but unit-tests-exemption label was set to ignore this.

Generated by 🚫 Danger

Comment thread tv/src/main/java/au/com/shiftyjelly/pocketcasts/search/TvSearchViewModel.kt Outdated
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
@sztomek sztomek added [Type] Feature Adding a new feature. [Area] Analytics Analytics related issues [Area] Search [Area] TV labels Aug 25, 2026
@sztomek sztomek added this to the 8.20 milestone Aug 25, 2026
@sztomek
sztomek marked this pull request as ready for review August 25, 2026 15:17
@sztomek
sztomek requested a review from a team as a code owner August 25, 2026 15:17
@sztomek
sztomek requested review from geekygecko and removed request for a team August 25, 2026 15:17
@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

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


Code review

  • Gather context (read current diff + surrounding code)
  • Re-check the previously-flagged findings against Address PR comments
  • Look for new issues in the latest state
  • Post findings

Re-reviewed at 0b71717. Four of the five earlier findings are resolved, cleanly. Two new (minor) points below, both posted inline.

Previous findings — status

# Finding Status
1 search_filter_tapped unfireable on focus-to-select pills Accepted as-is, with the sparseness caveat now documented in the PR description — that was the stated alternative, so this is settled
2 Navigation callbacks silently defaulted FixedonPodcastResultClick / onDiscoverPodcastClick / onDiscoverEpisodePodcastClick / onDiscoverCategoryClick are now required and sit above modifier; only onDiscoverListImpression keeps its default, which is right since it's the one that's analytics-only (TvSearchScreen.kt:226-233)
3 No search-side discover coverage Fixed — three new tests assert source = "search" on trackDiscoverListShown / trackDiscoverPodcastTapped / trackDiscoverEpisodePodcastTapped, plus search does not suppress the home local row ids pinning the localRowIds = emptySet() choice rather than leaving it incidental
4 localRowIds defaulted to emptySet() Fixed — now a required constructor param (TvDiscoverFeedAnalytics.kt:20), with the search call site spelling it out
5 Episode search_result_tapped baked into playEpisode Fixed — extracted to trackEpisodeResultTapped and called from the screen alongside playEpisode, matching the podcast half

New points (both minor, both inline)

Search's discover onCategoryClick can't fire todayinline. buildRows(discover, isLoggedIn) leaves includeHomeSections = false, and TvDiscoverFeedLoader.kt:114 drops ListType.Categories rows in that mode. So of the two sources the description lists for discover_categories_pill_tapped, only the browse-categories row (TvSearchScreen.kt:425) is reachable. Wiring it anyway is correct forward-looking; just don't expect two feeds of the event.

Episode-actions path emits nothinginline. onOpenEpisodeActions fires no event, and the modal's onGoToPodcast (TvSearchScreen.kt:202-205) opens a podcast with no event — the only "opened a podcast from Search" route in the file that's now untracked. Fine if Apple TV also skips it; flagging because it's the one remaining asymmetry.

Nit: categories.indexOfFirst { it.id == category.id } (TvSearchScreen.kt:425) is evaluated at click time, so a category that has left the list between composition and the click reports index = -1. Vanishingly unlikely on this screen; noting only because the discover-row equivalent (TvDiscoverRows.kt:132) has the same shape, so it's a consistent pattern rather than something to change here alone.

Verified in this pass

  • The TvHomeViewModel refactor is still purely delegation — every extracted method maps 1:1 onto a tracker call with SOURCE_HOME / LOCAL_ROW_IDS preserved, and UNKNOWN_VALUE moved with trackSponsoredPodcastTapped. trackBannerTapped correctly stayed on the tracker (home uses it) while the unused search-side delegate was dropped in 08c86c8.
  • SourceViewType.Search is the right type, not a string — SourceView.analyticsValue is declared SourceViewType (modules/services/analytics/.../SourceView.kt:7), so TV passing the enum directly matches what the phone app passes.
  • search_failed can't fire on a cancelled search: CancellationException is rethrown by the branch above it, and getOrThrow() re-surfaces what runCatching swallowed.
  • search_empty_results can't fire when partial results were shown — podcasts at line 176 is a superset of earlyPodcasts, so the empty check is consistent with what was displayed.
  • CallOnce { trackSearchShown() } matches how TvHomeScreen:68 and TvPlaylistDetailsScreen:96 already fire their shown events (rememberSaveable-backed, once per screen instance).
  • TvCategoryPodcastsScreen.kt:138-141 calls onPodcastClick and onOpenPodcast from the same click, so the tracking-only param can't desync from navigation.
  • Search feed contains no Banner or local rows either (includeHomeSections = false), so the tracker's discoverListId() gate is doing nothing on Search — which is exactly what the new test asserts.

I did not build the module or run the tests — no Gradle dependency cache in this environment — so this remains static analysis. CI here shows Gradle wrapper validation green; the Buildkite unit-test and spotlessCheck jobs are the ones to watch.

Nothing blocking from my side.
· branch feat/tv-analytics-search

@sztomek sztomek mentioned this pull request Aug 25, 2026
6 tasks

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

Clicking (select button) a filter pill logs search_filter_tapped; merely moving focus across pills does not.

On tvOS, as you move between the search chips "Top Results", "Podcasts", and "Episodes" they fire these events.

Screenshot 2026-08-26 at 8 48 33 pm

@sztomek
sztomek force-pushed the feat/tv-analytics-search branch from 0b71717 to c071478 Compare August 27, 2026 07:09
@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @sztomek's task in 1m 40s —— View job


Code review

Reviewed 3868d3c..c071478. All four remaining findings from the last pass are now addressed — localRowIds and the four navigation-bearing callbacks are required, episode search_result_tapped is extracted to trackEpisodeResultTapped and called from both the play path and the actions-modal Go to podcast path, and the search-side source="search" discover tests are in. I found no new defects; the episode.uuid used in the modal is the same uuid hydrate looked the PodcastEpisode up by, so the event carries the search result's uuid as intended.

Non-blocking

  • tv/src/main/java/au/com/shiftyjelly/pocketcasts/search/TvSearchViewModel.kt:197 — the filter-on-focus finding is still open, and @geekygecko's approval note reports that tvOS does fire search_filter_tapped while moving between the chips, which cuts against the "avoid logging pills merely passed over" rationale in the description. Either adopt the focus-settle debounce sketched in the original thread or say in the description that Android TV deliberately diverges from Apple TV here — the current text presents Apple TV as source of truth while emitting a strictly sparser signal.

I did not build the module or run the tests (no Gradle dependency cache in this environment), so this is static analysis only.
· branch feat/tv-analytics-search

@sztomek
sztomek force-pushed the feat/tv-analytics-search branch from c071478 to b06170b Compare August 27, 2026 07:14
@sztomek

sztomek commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Good catch @geekygecko — you're right, tvOS tracks this via onChange(of: model.scope) in SearchView.swift, so focus moves between the chips fire the event. I've switched Android TV to match: search_filter_tapped now fires whenever the selected filter changes (focus move or click), deduped so landing back on the already-selected pill doesn't re-fire. PR description updated.

@sztomek
sztomek force-pushed the feat/tv-analytics-search branch from b06170b to 4b494a4 Compare August 27, 2026 07:23
@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

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


Code review

Reviewed the new commit 4b494a4 on top of the rebase onto the folder-search feature. The filter rework is correct and closes the last open finding: onFilterSelected now fires search_filter_tapped on any actual filter change (focus or click) with a _filter.value dedup guard, matching tvOS's onChange(of: model.scope), and the internal reset in updateFolderResults correctly bypasses the event. trackEpisodeResultTapped(episodeUuid) taking a uuid instead of the item is fine — both call sites pass the same episode's uuid.

The rebase pulled in the new TvSearchFilter.Folders chip, and the analytics adaptation for it is incomplete.

Blocking

  • tv/src/main/java/au/com/shiftyjelly/pocketcasts/search/TvSearchViewModel.kt:377TvSearchFilter.Folders -> SearchResultFilterType.Unknown means focusing the Folders pill emits search_filter_tapped with filter = unknown; make analyticsValue nullable and skip the event for Folders, or add a real schema value. (thread)

Non-blocking

  • tv/src/main/java/au/com/shiftyjelly/pocketcasts/search/TvSearchScreen.kt:590 — opening a folder search result fires nothing, though SearchResultType.Folder exists and the phone app uses it (SearchFragment.kt:416); folder is now the only untracked result type on Search. Pre-existing from [TV] Local folder results in search  #5747, but this PR is what defines Search coverage.

I did not build the module or run the tests (no Gradle dependency cache in this environment), so this is static analysis only — in particular I could not confirm that SearchResultFilterType.Unknown is a no-arg member of the generated enum.
· branch feat/tv-analytics-search

@sztomek sztomek mentioned this pull request Aug 27, 2026
6 tasks
@sztomek
sztomek merged commit aaa01f9 into main Aug 27, 2026
26 checks passed
@sztomek
sztomek deleted the feat/tv-analytics-search branch August 27, 2026 08:03
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.

3 participants