Skip to content

[TV] Home analytics - #5734

Merged
sztomek merged 4 commits into
mainfrom
feat/tv-analytics-home
Aug 14, 2026
Merged

[TV] Home analytics#5734
sztomek merged 4 commits into
mainfrom
feat/tv-analytics-home

Conversation

@sztomek

@sztomek sztomek commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Description

First screen of the Android TV → Apple TV analytics parity program (EventHorizon only). Lays the analytics foundation for the TV app and completely mirrors the Home screen — on both platforms there is no separate Discover screen; the Discover feed is the Home tab body, so all its interactions are Home events (tvOS stamps them source = "home"). Based on feat/tv-search-category-details so the banner/category features exist to instrument.

Foundation

  • Adds :modules:services:analytics to the tv module and injects EventHorizon (the TV app previously fired no analytics).
  • Bumps the shared EventHorizon catalog 2026-07-142026-08-13_07-58-34, produced by Automattic/EventHorizonSchemas#118 (adds the android platform to the Apple-TV events, plus an optional source on the Discover list/category events). The bump is app-wide, so it adapts phone call sites whose event constructors changed: redirectPath = "none" on UserAccountCreatedEvent / UserSignedInEvent / SigninShownEvent, and source / isNewBookmark on BookmarkEditForm*Event.

Home analytics — verified against the tvOS app across every tracking layer (Analytics.track, AnalyticsHelper, DiscoverAnalytics, and the cell/model methods):

Event EventHorizon class Properties Fires when
home_shown HomeShownEvent Home shown (CallOnce)
discover_list_impression DiscoverListImpressionEvent list_id, source each Discover list row shown (local keep-listening / up-next / new-releases rows excluded)
discover_list_podcast_tapped DiscoverListPodcastTappedEvent list_id, podcast_uuid, source open any Discover podcast (incl. featured + category page)
discover_featured_podcast_tapped DiscoverFeaturedPodcastTappedEvent podcast_uuid open a featured podcast (also fires the row event)
discover_list_episode_tapped DiscoverListEpisodeTappedEvent list_id, podcast_uuid, episode_uuid, source play a Discover episode
discover_list_episode_play DiscoverListEpisodePlayEvent list_id, podcast_uuid play a Discover episode
discover_ad_category_tapped DiscoverAdCategoryTappedEvent name, region, id, podcast_id tap a sponsored podcast (also fires the row event; not on featured)
banner_row_tapped BannerRowTappedEvent type banner tap
discover_categories_pill_tapped DiscoverCategoriesPillTappedEvent name, region, index, visits, sponsored, source category pill tap

Analytics is added via additive no-op callbacks on the shared tvDiscoverRow / TvCategoryPodcastsScreen, so the Search screen is untouched (its analytics land in the Search PR, where it will stamp source = "search"). Subscribe actions (discover_featured_podcast_subscribed, discover_list_podcast_subscribed) fire from Podcast Details and are out of scope here.

Fidelity notes

  • source = "home" is stamped on discover_list_impression / discover_list_podcast_tapped / discover_list_episode_tapped / discover_categories_pill_tapped, mirroring tvOS — enabled by the optional source added in Automattic/EventHorizonSchemas#118.
  • region is the resolved Discover region (settings.discoverCountryCode.value) on discover_categories_pill_tapped and both discover_ad_category_tapped variants. visits / sponsored come from DiscoverCategory; the category-page ad carries the real category name / id. The only remaining unknown is the list-context ad's id (0) — an injected sponsored podcast in a normal list has no category id, and tvOS has the same gap there (it only has item?.categoryID, and likewise hardcodes name = "unknown").

Fixes PCDROID-719 https://linear.app/a8c/issue/PCDROID-719/home-screen-analytics

Testing Instructions

  1. ./gradlew :tv:installDebug; open Home → confirm one home_shown and a discover_list_impression per Discover row in adb logcat (debug logs via LoggingAnalyticsListener), all with source = "home".
  2. Open a featured podcast → discover_list_podcast_tapped + discover_featured_podcast_tapped. Open a non-featured one → only discover_list_podcast_tapped.
  3. Open a sponsored podcast → discover_list_podcast_tapped + discover_ad_category_tapped; a sponsored featured one → no discover_ad_category_tapped.
  4. Play a Discover episode → discover_list_episode_tapped + discover_list_episode_play.
  5. Tap a banner → banner_row_tapped; tap a category pill → discover_categories_pill_tapped; open a podcast inside a category → discover_list_podcast_tapped (with the category's real list_id).
  6. Confirm local rows (keep listening / up next / new releases) fire no discover_list_* events.
  7. Phone regression: sign-in / account-creation / bookmark-edit analytics still fire.

Screenshots or Screencast

SCR-20260813-jrxt

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
  • All strings that need to be localized are in localization — N/A
  • 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 — see Automattic/EventHorizonSchemas#118

@wpmobilebot

wpmobilebot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Project dependencies changes

list
! Upgraded Dependencies
com.automattic:eventhorizon:pocket-casts-2026-08-13_07-58-34, (changed from pocket-casts-2026-07-14_22-15-37)
tree
 +--- project ':modules:features:account'
 |    \--- project ':modules:features:search'
 |         \--- project ':modules:services:analytics'
-|              +--- com.automattic:eventhorizon:pocket-casts-2026-07-14_22-15-37
+|              +--- com.automattic:eventhorizon:pocket-casts-2026-08-13_07-58-34
 |              +--- project ':modules:services:model'
-|              |    +--- com.automattic:eventhorizon:pocket-casts-2026-07-14_22-15-37 (*)
+|              |    +--- com.automattic:eventhorizon:pocket-casts-2026-08-13_07-58-34 (*)
 |              |    \--- project ':modules:services:utils'
 |              |         \--- project ':modules:services:payment'
-|              |              \--- com.automattic:eventhorizon:pocket-casts-2026-07-14_22-15-37 (*)
+|              |              \--- com.automattic:eventhorizon:pocket-casts-2026-08-13_07-58-34 (*)
 |              \--- project ':modules:services:preferences'
-|                   \--- com.automattic:eventhorizon:pocket-casts-2026-07-14_22-15-37 (*)
+|                   \--- com.automattic:eventhorizon:pocket-casts-2026-08-13_07-58-34 (*)
 \--- project ':modules:features:discover'
      \--- project ':modules:features:podcasts'
           \--- project ':modules:features:player'
                \--- project ':modules:features:transcripts'
                     \--- project ':modules:services:sharing'
-                         \--- com.automattic:eventhorizon:pocket-casts-2026-07-14_22-15-37 (*)
+                         \--- com.automattic:eventhorizon:pocket-casts-2026-08-13_07-58-34 (*)

@sztomek
sztomek force-pushed the feat/tv-analytics-home branch 2 times, most recently from e560903 to e542fc9 Compare August 13, 2026 08:12
@sztomek sztomek changed the title Mirror the Apple TV home screen analytics [TV] Home analytics Aug 13, 2026
@sztomek sztomek added this to the 8.19 milestone Aug 13, 2026
@sztomek sztomek added [Type] Feature Adding a new feature. [Area] Analytics Analytics related issues [Area] TV labels Aug 13, 2026
@sztomek
sztomek marked this pull request as ready for review August 13, 2026 08:37
@sztomek
sztomek requested a review from a team as a code owner August 13, 2026 08:37
@sztomek
sztomek requested review from geekygecko and removed request for a team August 13, 2026 08:37
@sztomek

sztomek commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@claude please review this

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown

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


Code review

  • Read the PR diff (origin/feat/tv-search-category-details...HEAD)
  • Review analytics fidelity vs. the described tvOS behaviour
  • Review the catalog bump call-site adaptations (phone regressions)
  • Review tests
  • Post findings

Nice, well-scoped foundation. The additive no-op callbacks on tvDiscoverRow / TvCategoryPodcastsScreen keep Search genuinely untouched, and discoverListId() being an exhaustive when over TvDiscoverRow (TvHomeViewModel.kt:251) means a new row type can't silently skip the local-row decision. Test coverage on the VM is good. A few things below, roughly in order of how much they'd affect the data.


1. Every TV event will be reported as platform: "phone"

TracksAnalyticsTracker.updatePredefinedEventProperties() stamps the platform property from Util.getAppPlatform():

// modules/services/analytics/.../TracksAnalyticsTracker.kt:101
PredefinedEventProperty.PLATFORM to when (Util.getAppPlatform(appContext)) {
    AppPlatform.Automotive -> "automotive"
    AppPlatform.Phone -> "phone"
    AppPlatform.WearOs -> "watch"
},

AppPlatform (modules/services/utils/.../AppPlatform.kt:5) has no TV entry, and Util.getAppPlatform() (Util.kt:37) falls through to AppPlatform.Phone for anything that isn't Automotive/WearOs — even though Util.isTv() already exists at Util.kt:32 and the TV manifest sets pocketcasts_tv (tv/src/main/AndroidManifest.xml:36). So once the release/prototype tracker modules bind TracksAnalyticsTracker, every event this PR adds lands in Tracks indistinguishable from a phone event.

This is arguably the most important thing to get right in the foundation commit of a parity program — analyses that segment by platform will silently double-count TV into phone, and it's much harder to fix retroactively than to land now. (Same enum feeds deviceType in PlaybackStatsSyncWorker.kt:47, which is pre-existing but would be fixed by the same change.) Needs a DeviceType TV value in the catalog; worth checking whether #118 already added one.

Fix this →

2. region is the stored preference, not the resolved Discover region

// TvHomeViewModel.kt:249
private fun discoverRegion(): String = settings.discoverCountryCode.value

The PR description calls this "the resolved Discover region", but the feed actually resolves with a fallback:

// TvDiscoverFeedLoader.kt:205
discover.regions[settings.discoverCountryCode.value] ?: discover.regions[discover.defaultRegionCode]

When the stored country code isn't present in the feed's regions, the user browses the default region while discover_categories_pill_tapped / discover_ad_category_tapped report the stored one. TvDiscoverFeedLoader already computes the resolved region — exposing it (e.g. caching the last resolved DiscoverRegion.code on the loader) would make the property match what was actually shown. Edge-casey, but it's a silent mismatch rather than a visible one.

3. discover_categories_pill_tapped.index means something different than on phone

TV sends the display position (row.categories.indexOf(category), TvDiscoverRows.kt:151). The phone sends the featured index:

// modules/features/discover/.../DiscoverFragment.kt:180
index = category.featuredIndex?.toLong() ?: -1,

featuredIndex is never populated on the TV feed path, so phone would emit -1 where TV emits 0..n. Parity with tvOS may well be the display index — but two Android surfaces disagreeing on the same property is worth a conscious decision (and a note for whoever queries it), not an accident.

4. Impressions are per-composition, not per-visit

// TvDiscoverRows.kt:39, 64, 89, 117
LaunchedEffect(row.id) { onListImpression(row) }

Inside item(key = row.id) this re-fires every time the row re-enters composition, so scrolling a row off-screen and back emits another discover_list_impression. LazyColumn's prefetch can also compose a row just before it becomes visible, so a row the user scrolls past quickly can be counted without being seen.

Both are probably parity with willDisplay on tvOS, so I'd call this "confirm intent" rather than a bug — but if the intended metric is one impression per row per Home visit, the VM would need a seen: MutableSet<String>. Also worth adjusting testing step 1 in the description: only the initially-visible rows fire on open, not one per Discover row.

5. trackDiscoverPodcastTapped doesn't early-return for local rows

// TvHomeViewModel.kt:193
fun trackDiscoverPodcastTapped(row: TvDiscoverRow, podcast: TvDiscoverPodcast) {
    row.discoverListId()?.let { listId -> ... }
    if (row is TvDiscoverRow.FeaturedPodcasts) { ... } else if (podcast.isSponsored) { ... }
}

The two episode trackers (:205, :211) bail out on a null list id; this one only skips the list event and still fires the featured/ad event. Unreachable today because all three local rows are Episodes, but the asymmetry is exactly the kind that breaks quietly if "keep listening" ever becomes a podcast row. A val listId = row.discoverListId() ?: return at the top would make all four consistent.

6. Smaller things

  • TvDiscoverRows.kt:151row.categories.indexOf(category) is a linear scan using structural equality inside the click lambda. TvRow already keys by it.id; indexOfFirst { it.id == category.id } would at least key on identity. Nit.
  • BookmarkViewModel.kt:126 vs :135/:144onShown takes isNewBookmark from the fragment args while onClose/onSubmitBookmark read uiState.value.isNewBookmark, which load() can flip when findByEpisodeTime resolves an existing bookmark at the same timestamp. A single form session can then emit shown(isNewBookmark = true)submitted(isNewBookmark = false). Deliberate (submitting genuinely updated an existing bookmark), or should all three come from the same source?
  • "none" redirect pathNO_REDIRECT_PATH is a constant in SyncManagerImpl.kt:111 but a bare literal in AccountActivity.kt:164 and OnboardingLogInViewModel.kt:128. One shared constant would keep the three in sync; alternatively, is a nullable/optional redirectPath in the schema more honest than a magic "none" on a platform that has no redirects?
  • TeststrackDiscoverEpisodePodcastTapped is the only tracker without a test, and nothing asserts that Banner/Categories rows produce no impression (the discoverListId() when covers it, but it's the branch most likely to be "fixed" wrongly later).

I reviewed statically and did not run a build or the unit tests here (Android build + the newly-bumped EventHorizon artifact), so I can't confirm the new event constructor signatures compile — CI covers that. Nothing above is blocking except possibly #1, which is worth deciding on before the rest of the parity program lands on top of it.
• branch feat/tv-analytics-home

@sztomek
sztomek force-pushed the feat/tv-search-category-details branch from 44cb8bc to e9ef950 Compare August 14, 2026 11:02
Base automatically changed from feat/tv-search-category-details to main August 14, 2026 12:10
@sztomek
sztomek force-pushed the feat/tv-analytics-home branch from e542fc9 to ae937f4 Compare August 14, 2026 12:18
@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

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


Code review

  • Read the updated PR diff
  • Re-check the findings from my previous review
  • Review analytics fidelity and the catalog-bump call sites
  • Post findings

Nothing changed in the code since my last review — the update was a rebase (the base feat/tv-search-category-details merged into main as a394ecc), so the diff is now against origin/main but the content is identical (356/-32, 16 files). So this is a re-check rather than a fresh pass. I dug further into two of the previous findings and both got stronger; details below.


1. Every TV event will be reported as platform: "phone" — still open, and it does reach Tracks

I verified the full chain this time rather than inferring it:

  • AppPlatform (modules/services/utils/.../AppPlatform.kt:5) has only Automotive / Phone / WearOs.
  • Util.getAppPlatform() (Util.kt:37) falls through to AppPlatform.Phone for anything not Automotive/WearOs, even though Util.isTv() exists at Util.kt:32 and tv/src/main/AndroidManifest.xml:36 sets pocketcasts_tv.
  • TracksAnalyticsTracker.kt:104 maps that enum to the platform predefined property with no TV case.
  • The tv module does build a release variant (tv/build.gradle.kts:33; only prototype is disabled), and ReleaseTrackerModule binds TracksAnalyticsTracker — so these events genuinely land in Tracks stamped platform = "phone", not just in the debug logger.

Same enum also feeds deviceType in PlaybackStatsSyncWorker.kt:47 (pre-existing, fixed by the same change). Worth landing in this foundation commit, before the rest of the parity program stacks on top — retroactively splitting TV out of phone data isn't possible. I couldn't confirm whether the bumped catalog has a DeviceType.Tv (the artifact isn't in the local Gradle cache), so that needs checking first.

Fix this →

2. region disagrees with the phone — stronger than I first put it

// TvHomeViewModel.kt:249
private fun discoverRegion(): String = settings.discoverCountryCode.value

I'd flagged this as "stored vs. resolved". It's also a cross-surface inconsistency on the same property: the phone sends the resolved region for the identical event —

// DiscoverFragment.kt:179
region = viewModel.currentRegionCode.orEmpty(),
// DiscoverViewModel.kt:146 — assigned after resolution
currentRegionCode = region.code

whereas TvDiscoverFeedLoader.kt:205 resolves with discover.regions[settings.discoverCountryCode.value] ?: discover.regions[discover.defaultRegionCode] and the VM never sees the fallback. So when the stored code isn't in the feed's regions, TV and phone report different region values for the same user in the same state. Exposing the resolved DiscoverRegion.code from the loader would fix it.

3. discover_categories_pill_tapped.index still means something different than on phone

TV sends the display position (row.categories.indexOf(category), TvDiscoverRows.kt:151); phone sends category.featuredIndex ?: -1 (DiscoverFragment.kt:180), and featuredIndex is never populated on the TV feed path. Parity with tvOS may well be the display index — just worth a conscious decision plus a note for whoever queries it, since the two Android surfaces will disagree.

4. Impressions re-fire on every tab switch, not just on scroll

I said "per-composition" last time; I can now be concrete about the main trigger. TvScaffold.kt:164 renders tab content in a Crossfade with no SaveableStateHolder, so leaving the Home tab disposes TvHomeScreen entirely. Coming back re-composes it, which re-runs CallOnce { trackHomeShown() } (its rememberSaveable state is discarded with the composition) and every LaunchedEffect(row.id) { onListImpression(row) } for the visible rows.

That's plausibly correct parity with tvOS viewDidAppear/willDisplay — but it means home_shown is per-tab-visit rather than per-session, and impressions are per-visit-per-composition. Please confirm that's intended; if the metric is one impression per row per Home visit, the VM needs a seen: MutableSet<String>. Either way, testing step 1 in the description should say only the initially visible rows fire on open.

5. trackDiscoverPodcastTapped doesn't early-return for local rows

// TvHomeViewModel.kt:193
row.discoverListId()?.let { listId -> ... }
if (row is TvDiscoverRow.FeaturedPodcasts) { ... } else if (podcast.isSponsored) { ... }

The two episode trackers (:205, :211) bail on a null list id; this one only skips the list event and still fires featured/ad. Unreachable today (all three local rows are Episodes), but val listId = row.discoverListId() ?: return would make all four consistent.

6. Smaller things

  • TvDiscoverRows.kt:151row.categories.indexOf(category) is a linear scan on structural equality inside the click lambda; indexOfFirst { it.id == category.id } matches how TvRow already keys (key = { it.id }). Nit.
  • BookmarkViewModel.kt:126 vs :135/:144 — confirmed the asymmetry is real: isNewBookmark is derived from UiState.bookmarkUuid (:52), and load() overwrites bookmarkUuid when findByEpisodeTime resolves an existing bookmark at the same timestamp (:73). So onShown(isNewBookmark = args.bookmarkUuid == null) can emit true and the matching submitted/dismissed emit false for one form session. Deliberate, or should all three read the same source?
  • "none" redirect path — a named constant in SyncManagerImpl.kt:111 but a bare literal in AccountActivity.kt:164 and OnboardingLogInViewModel.kt:128. One shared constant would keep them in sync; alternatively a nullable redirectPath in the schema is more honest for a platform with no redirects.
  • discover_list_episode_play fires on tap, not on playbackonEpisodePlay(row, episode) runs before onPlayEpisode(episode) (TvDiscoverRows.kt:105), and playEpisode can fail and surface an error toast (TvHomeViewModel.kt:274). The _tapped event is right either way; whether _play should follow _playStarted depends on what tvOS does.
  • TeststrackDiscoverEpisodePodcastTapped is still the only tracker without a test, and nothing asserts that Banner/Categories rows produce no impression.

Same bottom line as before: nothing here blocks except possibly #1, which is cheap now and expensive later. I reviewed statically — no build or unit-test run in this job, so the new EventHorizon constructor signatures are unverified locally; CI covers that.
• branch feat/tv-analytics-home

@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

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


Code review — re-check after 398cbea "Address PR comments"

  • Read the new commit
  • Verify the platform fix end-to-end
  • Re-check the previously-raised findings
  • Post findings

398cbea addresses four of the earlier points. Verified each:

Earlier finding Status
#1 TV events reported as platform: "phone" FixedTracksAnalyticsTracker.kt:105 now branches on Util.isTv first. Confirmed the value reaches Tracks: tv/build.gradle.kts:33 builds a release variant and ReleaseTrackerModule.kt:18 binds TracksAnalyticsTracker.
#5 trackDiscoverPodcastTapped didn't early-return for local rows FixedTvHomeViewModel.kt:194 is now ?: return, consistent with the other three trackers.
#6 indexOf linear scan on structural equality FixedindexOfFirst { it.id == category.id } (TvDiscoverRows.kt:151).
Missing tests FixedtrackDiscoverEpisodePodcastTapped now covered, plus a verifyNoInteractions test asserting Banner/Categories rows fire no impression.

Two follow-ups on the platform fix, then the items still open.


1. AppPlatform still maps TV → DeviceType.Phone (latent, not live)

The fix bypasses AppPlatform rather than extending it, so AppPlatform.kt still has no TV entry and AppPlatform.analyticsValue (an EventHorizon DeviceType) still resolves to DeviceType.Phone on TV. The one analytics consumer is PlaybackStatsSyncWorker.kt:47 (deviceType on ListeningTimeEvent).

I checked whether that's reachable: the worker is only scheduled from app/.../PocketCastsApplication.kt:320-321, so no TV data is wrong today — correcting my earlier framing, this isn't a live bug. But the two now disagree by construction: platform = "tv" on every event while device_type would say phone the moment listening-stats sync is enabled on TV. A comment at AppPlatform.kt noting that TV is deliberately handled outside the enum (or adding the enum value once the catalog has DeviceType.Tv) would stop that from being rediscovered later.

Two smaller things in the same hunk:

  • Util.isTv(appContext) does a PackageManager.getApplicationInfo(GET_META_DATA) IPC on every call, unlike getAppPlatform which caches (Util.kt:37). It runs once per refreshMetadata(), so it's not hot — but folding the TV check into the cached getAppPlatform would both avoid the IPC and keep one source of truth for "what platform is this".
  • The nested when {} → when () {} with a blank line between the two branches reads awkwardly for what is conceptually one four-way choice; a flat when { Util.isTv(..) -> "tv"; isAutomotive -> ... } or the getAppPlatform route would be tidier. Style only.

Worth confirming with whoever owns the Tracks schema that "tv" is an accepted value for platform — dashboards that filter on an enumerated set will silently drop unknown values.


Still open (unchanged — flagging so they're an explicit decision, not an oversight)

region is the stored preference, not the region actually browsedTvHomeViewModel.kt:249 sends settings.discoverCountryCode.value, but TvDiscoverFeedLoader.kt:204-207 resolves with discover.regions[stored] ?: discover.regions[defaultRegionCode]. The phone sends the resolved code for the same event (DiscoverFragment.kt:179DiscoverViewModel.kt:146), so when the stored code isn't in the feed the two Android surfaces report different region for the same user. resolveRegionOrNull already computes the right value; exposing the last-resolved DiscoverRegion.code from the loader would close it.

Fix this →

discover_categories_pill_tapped.index means something different than on phone — TV sends the display position, phone sends category.featuredIndex ?: -1 (DiscoverFragment.kt:180) and featuredIndex is never populated on the TV feed path. Fine if tvOS parity is the display index — just needs to be a conscious choice plus a note for whoever queries it.

home_shown and impressions re-fire on every Home-tab visitTvScaffold.kt:164 renders tab content in a Crossfade with no SaveableStateHolder, so leaving Home disposes TvHomeScreen; returning re-runs CallOnce { trackHomeShown() } and each LaunchedEffect(row.id) { onListImpression(row) } for the visible rows. Plausibly correct parity with tvOS viewDidAppear/willDisplay — please confirm. Either way, testing step 1 in the description should say only the initially visible rows fire on open, not one per Discover row.

BookmarkViewModel isNewBookmark sources disagreeonShown takes it from the fragment args (BookmarkFragment.kt:60) while onClose/onSubmitBookmark read uiState.value.isNewBookmark, which is derived from bookmarkUuid (:52) and gets overwritten in load() when findByEpisodeTime resolves an existing bookmark at the same timestamp (:77-81). One form session can emit shown(isNewBookmark = true)submitted(isNewBookmark = false). Deliberate?

"none" redirect path — now a named constant in SyncManagerImpl.kt:111 but still a bare literal in AccountActivity.kt:164 and OnboardingLogInViewModel.kt:128.

discover_list_episode_play fires on tap, not on playback startonEpisodePlay(row, episode) runs before onPlayEpisode(episode) (TvDiscoverRows.kt:105), and playEpisode can fail into an error toast. Depends on what tvOS does.


Nothing blocking. The one that mattered most is fixed and I verified the wiring; the rest are consistency/intent calls. Reviewed statically — no build or test run in this job, so the bumped EventHorizon constructor signatures remain CI-verified only.
• branch feat/tv-analytics-home

Comment thread tv/src/main/java/au/com/shiftyjelly/pocketcasts/discover/TvDiscoverRows.kt Outdated
@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

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


Code review — re-check after be1d3b4 "Address PR comments"

  • Read the new commit
  • Verify the callback merge for regressions
  • Re-check previously-raised findings
  • Respond to @geekygecko's review comments
  • Post findings

be1d3b4 collapses the paired tracking/navigation callbacks that @geekygecko asked about — tvDiscoverRow now takes one onPodcastClick(row, podcast) / onEpisodePlay(row, episode) / onEpisodePodcastClick(row, episode) / onCategoryClick(category, index) each, and the Home screen does track-then-navigate in a single lambda. I traced every call site (TvHomeScreen.kt:96-112, TvSearchScreen.kt:311-322) and the behaviour is preserved on both screens; making the four callbacks required is a nice touch, since a new caller can't now silently drop tracking. Two new things below, then the items still open.


1. category lambda parameter shadows the outer val category — likely a build break

// TvHomeScreen.kt:70
val category = openedCategory
...
// TvHomeScreen.kt:108
onCategoryClick = { category, index ->
    viewModel.trackCategoryPillTapped(category, index)
    openedCategory = TvOpenedCategory(category.id, category.name, category.source)
},

The outer category is still live below (:116 category != null, :120 target = category), so this is a genuine shadow. Kotlin reports NAME_SHADOWING for lambda parameters, and this repo sets allWarningsAsErrors.set(true) for every subproject (build.gradle.kts:212) — the codebase already carries @Suppress("NAME_SHADOWING") in a dozen places (OnboardingLoginPage.kt:93, Util.kt:60, …) precisely because of that. Renaming the parameter (tappedCategory) is cleaner than suppressing. I couldn't compile here to confirm, so if the Buildkite compile is already green, ignore this.

Fix this →

2. TvCategoryPodcastsScreen still has the double-callback pattern the commit removed everywhere else

// TvCategoryPodcastsScreen.kt:139
onPodcastClick(state.listId, podcast)
onOpenPodcast(podcast.uuid)

…driven by two separate params (:39 onOpenPodcast, :42 onPodcastClick) wired from TvHomeScreen.kt:129-130. That's exactly the shape @geekygecko flagged, just in the other file the PR touches. Same treatment — one onPodcastClick: (String?, TvDiscoverPodcast) -> Unit that the Home screen implements as track-then-navigate — would make the two screens consistent. Note onPodcastClick here is still an optional no-op default, unlike the now-required ones on tvDiscoverRow.

Minor, same file family: onTapBanner / onListImpression on tvDiscoverRow (TvDiscoverRows.kt:31-32) remain optional no-ops while the other four are required, so Search silently emits no impressions. That's the intended scoping for this PR — worth a one-line comment saying so, since the asymmetry now reads as an oversight rather than a decision.


On @geekygecko's redirectPath comment

Agreed, and it's the cleaner fix for the thing I flagged earlier as a stringly-typed "none" scattered across three call sites (SyncManagerImpl.kt:111 as a constant, AccountActivity.kt:164 and OnboardingLogInViewModel.kt:128 as bare literals). Making redirect_path optional in the schema and omitting it on Android is more honest than a magic sentinel that every query then has to know to filter out — a Web-Player-only property genuinely has no value on a platform with no redirects. It does need a follow-up EventHorizonSchemas PR before this can land, so it's a "next catalog bump" item rather than something to block on.


Still open from earlier passes (unchanged in be1d3b4)

region is the stored preference, not the region actually browsedTvHomeViewModel.kt:249 sends settings.discoverCountryCode.value; TvDiscoverFeedLoader.kt:204-207 resolves with discover.regions[stored] ?: discover.regions[defaultRegionCode]. Phone sends the resolved code for the same event (DiscoverFragment.kt:179DiscoverViewModel.kt:146), so when the stored code isn't in the feed the two Android surfaces disagree on region for the same user.

Fix this →

discover_categories_pill_tapped.index means something different than on phone — TV sends display position (TvDiscoverRows.kt:132), phone sends category.featuredIndex ?: -1 (DiscoverFragment.kt:180), and featuredIndex is never populated on the TV feed path. Fine if tvOS parity is the display index — just needs to be a conscious call plus a note for whoever queries it.

home_shown and impressions re-fire on every Home-tab visitTvScaffold.kt:164 renders tab content in a Crossfade with no SaveableStateHolder, so leaving Home disposes TvHomeScreen; returning re-runs CallOnce { trackHomeShown() } and each LaunchedEffect(row.id) { onListImpression(row) } for the visible rows. Plausibly correct parity with tvOS viewDidAppear/willDisplay — please confirm. Either way, testing step 1 in the description should say only the initially visible rows fire on open.

BookmarkViewModel isNewBookmark sources disagreeonShown takes it from the fragment args (BookmarkFragment.kt:60) while onClose/onSubmitBookmark read uiState.value.isNewBookmark, derived from bookmarkUuid (:52) which load() overwrites when findByEpisodeTime resolves an existing bookmark at the same timestamp. One form session can emit shown(true)submitted(false). Deliberate?

discover_list_episode_play fires on tap, not on playback startonEpisodePlay now tracks then calls viewModel.playEpisode (TvHomeScreen.kt:100-103), and playEpisode can fail into an error toast (TvHomeViewModel.kt:274). The _tapped event is right either way; whether _play should follow playStarted depends on tvOS.

AppPlatform still has no TV entry — the platform fix in 398cbea branches on Util.isTv inside TracksAnalyticsTracker rather than extending the enum, so AppPlatform.analyticsValue still resolves to DeviceType.Phone on TV. Not reachable today (its only consumer, PlaybackStatsSyncWorker, is scheduled from the phone app only), but platform = "tv" and device_type = phone disagree by construction the moment stats sync reaches TV. A comment on the enum would stop that being rediscovered later.


Nothing blocking except #1 if it does break the compile. The refactor in this commit is a clear improvement over the additive-callback approach — good call from @geekygecko. Reviewed statically; no build or test run in this job.
• branch feat/tv-analytics-home

@sztomek
sztomek merged commit 8f0e6fd into main Aug 14, 2026
21 checks passed
@sztomek
sztomek deleted the feat/tv-analytics-home branch August 14, 2026 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Area] Analytics Analytics related issues [Area] TV [Type] Feature Adding a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants