[TV] Episode actions & details analytics - #5741
Conversation
1f4ed8b to
39b7a03
Compare
a62b7eb to
a6a530a
Compare
39b7a03 to
f0be555
Compare
a6a530a to
6af74bc
Compare
|
@claude please review this |
|
Claude finished @sztomek's task in 3m 49s —— View job Code review
Overall this is clean and follows the conventions the earlier PRs in this series established ( Findings1. PR description "decision 2" doesn't match the diff — inline 2. 3. 4. Now Playing can emit an extra 5. Test coverage. The two new tests assert one-line pass-throughs. The part that can actually be wrong — the Verified as correct
I did not run Gradle (build/tests run on Buildkite for this repo), so the compile and unit-test results here are from reading only. |
|
Claude finished @sztomek's task in 3m 17s —— View job Code review
The wiring is clean and consistent with the pattern established in the earlier PRs of this program (
Four things worth addressing, one of which is in the PR description rather than the code. 1. The PR description's "Decision 2" doesn't match the diff
None of that happened in this diff. On the base branch, SearchResults(SourceView.SEARCH_RESULTS),and Since the PR body is the durable record for an analytics change, this is worth correcting so nobody later goes hunting for a 2. The surface →
|
| call site | actions context | details literal |
|---|---|---|
TvPodcastDetailsScreen.kt:335/346 |
PodcastDetails |
PodcastScreen |
TvPlaylistDetailsScreen.kt:326/338 |
Playlist |
Filters |
TvSearchScreen.kt:156/171 |
SearchResults |
Search |
TvUpNextScreen.kt:175/187 |
UpNext |
UpNext |
TvNowPlayingScreen.kt:348/360 |
NowPlaying |
NowPlaying |
Nothing stops the two halves of a pair from drifting apart — an added surface, or a copy-paste when adding a sixth screen, and episode_actions_shown and episode_detail_shown silently report different sources for the same episode row. Taking actionContext: TvEpisodeActionContext in TvEpisodeInfoModal (and using actionContext.episodeViewSource internally) makes the enum the single source of truth and makes the pair impossible to mismatch. Fix this →
3. The two modals use different LaunchedEffect keys, so they behave differently when the episode changes underneath
TvEpisodeActionsModal.kt:66→LaunchedEffect(Unit)TvEpisodeInfoModal.kt:65→LaunchedEffect(episode.uuid)
For 4 of the 5 surfaces the episode is pinned by local state, so this is a distinction without a difference. But in Now Playing the episode is the live playback state (TvNowPlayingScreen.kt:344-363): if playback auto-advances while a modal is open, the details modal fires a second episode_detail_shown while the actions modal — whose displayed episode also just changed — fires nothing. Keying both on episode.uuid makes the two events agree, and uuid is the right key regardless since a progress tick produces a new PodcastEpisode instance with the same uuid.
4. The new tests assert the forwarding, not the decision
Both tests are effectively vm.trackX(source) → verify(eventHorizon).track(Event(source)). That's a one-line pass-through; the part of this PR that can actually be wrong is the mapping — which surface yields which source — and that's untested on both sides (the enum table, and 5 hardcoded literals). A parameterized assertion over TvEpisodeActionContext.entries checking source/episodeViewSource pairs would be cheap and would guard the thing reviewers are being asked to verify event-by-event against tvOS. Folding in #2 would extend that coverage to the details modal for free.
Nits
TvEpisodeActions.trackActionsShownvsTvEpisodeInfoViewModel.trackShown—trackDetailShownwould read symmetrically with the event names.TvEpisodeActionsViewModel.kt:53places the newtrackActionsShownoverride aboveplay, ahead of the other interface overrides which otherwise follow the interface's declaration order.
Not verified: I reviewed statically and did not run ./gradlew :tv:testDebugUnitTest or spotlessCheck in this environment — the com.automattic:eventhorizon artifact isn't in the local cache here, so I couldn't confirm the EpisodeViewSourceType member names (PodcastScreen/Search/Filters/UpNext/NowPlaying) against the library; CI compilation will.
· branch feat/tv-analytics-episode-actions
25389c3 to
cc64d92
Compare
6af74bc to
e022200
Compare
|
Claude finished @sztomek's task in 3m 57s —— View job Code review — re-review after
|
| Earlier point | Status |
|---|---|
source plumbed two different ways |
✅ TvEpisodeInfoModal now takes actionContext: TvEpisodeActionContext (TvEpisodeInfoModal.kt:62) and derives actionContext.episodeViewSource internally; all 5 call sites pass the same context they already pass to the sibling actions modal, and the EventHorizon import is gone from the 5 UI files |
LaunchedEffect(Unit) vs LaunchedEffect(episode.uuid) |
✅ both modals now key on episode.uuid (TvEpisodeActionsModal.kt:66, TvEpisodeInfoModal.kt:67) |
| Mapping table untested | ✅ TvEpisodeActionTypeTest.kt:11-27 asserts both columns for every entry, and assertEquals(TvEpisodeActionContext.entries.toSet(), expected.keys) makes a newly-added context fail the test rather than silently skip it — exactly the guard that was missing |
trackShown naming / override placement |
✅ renamed to trackDetailShown; the override now sits after removeFromUpNext, matching interface order |
I was also able to confirm what the last round couldn't: the EpisodeViewSourceType member names are all real — PodcastScreen, Search, Filters, UpNext, NowPlaying are each already referenced from modules/services/model/.../EpisodeViewSource.kt.
No correctness bugs, no security or performance concerns — two track calls on an already-injected singleton. Four things left, none of them blocking.
1. The PR description's "Decision 2" is still inaccurate — and the new test now contradicts it
The body says:
Previously the Search screen borrowed
TvEpisodeActionContext.PodcastDetails… Adding aSearchcontext (withSourceView.SEARCH) both gives the new shown-event the rightsourceand corrects those existing playback events tosource=search.
The code says otherwise. TvEpisodeActionsViewModel.kt:24:
SearchResults(SourceView.SEARCH_RESULTS, EpisodeViewSourceType.Search),SearchResults(SourceView.SEARCH_RESULTS) existed on the base branch, TvSearchScreen.kt:155 already used it, and the only change is the second column. And the new mapping test now asserts SourceView.SEARCH_RESULTS explicitly (TvEpisodeActionTypeTest.kt:15) — so the durable record and the test disagree in writing. No existing playback event changes its source; they were search_results before and still are. Worth a description edit before merge, since this is an analytics PR whose body is what reviewers verify against tvOS. (Decision 1 — deliberately firing episode_actions_shown from Search where Apple TV doesn't — is accurate.)
2. No episode_detail_dismissed counterpart (inline)
Mobile fires EpisodeDetailShownEvent and EpisodeDetailDismissedEvent as a pair (EpisodeFragment.kt:358 / :368). TV emits only the shown half, so anything pairing the two sees unmatched TV events. Probably intentional given the stated scope, but it belongs in the PR body rather than being discovered later.
3. Config-change re-fire (inline)
Mobile guards both events with if (!viewModel.isFragmentChangingConfigurations). LaunchedEffect(episode.uuid) re-runs on activity recreation with a modal open (locale change, dark-mode toggle, display change). Far rarer on TV than on a phone — flagging only because the two platforms will count it differently.
4. Now Playing still double-counts on auto-advance
Now Playing is the only surface where episode is live playback state rather than captured local state (TvNowPlayingScreen.kt:343-362), so if playback advances while a modal is open, episode.uuid changes and a second event fires for an episode the user never opened. Unchanged from the last round, and now symmetric across both modals (the actions modal keys on uuid too), which is arguably the better trade — one small skew in now_playing on both events beats an inconsistency between them. Fine to accept; capturing the episode at open time is the fix if it ever matters. Fix this →
Nit — a canonical surface → EpisodeViewSourceType mapping already exists
modules/services/model/.../EpisodeViewSource.kt already maps app surfaces to EpisodeViewSourceType (PODCAST_SCREEN, FILTERS, UP_NEXT, SEARCH, NOW_PLAYING are all there), and mobile tracks via episodeViewSource.analyticsValue. TvEpisodeActionContext now holds a second, parallel copy of five of those rows. Holding EpisodeViewSource and reading .analyticsValue would keep one table for the whole app. Not worth churn if the TV enum is expected to stay this small.
Verified as correct
- All 5 details-modal call sites pass a context matching their sibling actions modal —
PodcastDetails(TvPodcastDetailsScreen.kt:334/345),Playlist(:325/337),SearchResults(:155/170),UpNext(:175/187),NowPlaying(:347/359). With the mapping now living in the enum, these can't drift. - Tracking from the shared modal rather than per call site means a sixth surface gets both events for free.
LaunchedEffect(episode.uuid)sits above thependingConfirmationbranch inTvEpisodeActionsModal, so the confirm → cancel round trip doesn't re-fire.NoOpTvEpisodeActionsimplementstrackActionsShown, so previews stay side-effect free;TvEpisodeInfoModal's preview targetsTvEpisodeInfoModalContentand is unaffected.- Tapping Episode details clears
actionsEpisodeat every call site and dismissing details never reopens actions, so the flow in the testing instructions yields exactly one of each event. - Reopening the same episode's details re-fires the event even though
TvEpisodeInfoViewModel.load()early-returns on the same uuid — the tracking call is beforeload(), which is what you want. - Import ordering matches the surrounding files.
Not verified: I reviewed statically. Gradle isn't runnable in this environment (./gradlew is not in the allowed tools here), so :tv:compileDebugKotlin, :tv:testDebugUnitTest and spotlessCheck results are from reading only — CI will confirm.
· branch feat/tv-analytics-episode-actions
|
|
||
| fun trackDetailShown(source: EpisodeViewSourceType) { | ||
| eventHorizon.track(EpisodeDetailShownEvent(source = source)) | ||
| } |
There was a problem hiding this comment.
Two things worth comparing against the existing mobile implementation of this same event, EpisodeFragment.kt:353-361:
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
if (!viewModel.isFragmentChangingConfigurations) {
eventHorizon.track(EpisodeDetailShownEvent(source = episodeViewSource.analyticsValue))
}
}
override fun onDestroyView() {
...
if (!viewModel.isFragmentChangingConfigurations) {
eventHorizon.track(EpisodeDetailDismissedEvent(source = episodeViewSource.analyticsValue))
}-
No
episode_detail_dismissedcounterpart. Mobile emits shown/dismissed as a pair, so anything that pairs them (dwell time, "opened details but bounced") will see TVepisode_detail_shownevents with no matching dismiss. If that's deliberate — tvOS doesn't fire it either, and the PR is explicitly scoped to the two shown events — it's worth a line in the PR body, since that's the record the analytics folks read.TvEpisodeInfoModal.onDismissRequestis the natural hook if it's wanted. -
Config-change guard. Mobile suppresses the re-fire when the fragment is recreated for a configuration change.
LaunchedEffect(episode.uuid)here re-runs whenever the activity is recreated with the modal open (locale change, dark-mode toggle, display/HDMI change on TV). Much rarer on TV than on a phone, so probably fine to accept — just flagging that the two platforms count this differently.
|
Version |
Description
Fifth screen-area of the Android TV → Apple TV analytics parity program (EventHorizon only): the shared episode options modal and episode details modal. Stacked on the Up Next analytics PR. Verified event-by-event against the tvOS app.
episode_actions_shownEpisodeActionsShownEventsourceTvEpisodeActionsModal) appearsepisode_detail_shownEpisodeDetailShownEventsourceTvEpisodeInfoModal) appearstvOS fires these bare (
EpisodeRow.swift:292.onAppearandEpisodeShowNotesView.swift:26.task, no properties), but both Android EventHorizon classes require asource: EpisodeViewSourceType. We map it from the surface the modal is shown on:sourcepodcast_screenfiltersup_nextnow_playingsearchWired the established way:
EventHorizoninjected intoTvEpisodeActionsViewModel(newtrackActionsShown(source)on theTvEpisodeActionsinterface) andTvEpisodeInfoViewModel(newtrackDetailShown(source)); each fires once from the modal'sLaunchedEffect(episode.uuid)on appear. Thesourcemapping lives in one place — the newepisodeViewSourcecolumn on theTvEpisodeActionContextenum — and both modals derive it from theTvEpisodeActionContextpassed at each of the 5 call sites, so the two events can't drift apart.Two decisions worth calling out
1.
episode_actions_shownfrom Search — intentionally fired (diverges from Apple TV).Apple TV's Search/Discover results use a reduced menu (
DiscoveryEpisodeMenuButtons) that has no tracking, so Apple TV does not emitepisode_actions_shownthere. Android TV reuses the fullTvEpisodeActionsModalfor search results, and we chose to fire the event from Search too, stampedsource=search. This is a deliberate divergence for completeness of Android's own analytics — noted here so it's not mistaken for an accidental over-fire.2. The
sourcemapping lives only in theTvEpisodeActionContextenum.Each context already carried a
SourceView(used for playback actions); this PR adds anepisodeViewSource: EpisodeViewSourceTypecolumn alongside it for the two new shown-events. Both the actions modal and the details modal read theirsourcefrom the sameTvEpisodeActionContext, so a screen can't reportepisode_actions_shownandepisode_detail_shownwith mismatched sources, and a future surface only has to add one enum entry. No existing playbacksourcechanges here — Search already usedTvEpisodeActionContext.SearchResults(SourceView.SEARCH_RESULTS) on the base branch.Not in scope:
up_next_queue_reorderedApple TV fires
up_next_queue_reorderedfrom the shared Play Next / Play Last helper, but only when the episode is already in Up Next (a move). Android'sPlaybackManager.playNext/playLastinstead unconditionally fire a different event (episode_added_to_up_next) with no move-detection, and the reorder event is fired nowhere in the Android codebase. Layering iOS's semantics on top would double-count and needs aslotsvalue iOS never sends — so it's deliberately deferred as its own decision.Fixes PCDROID-724 https://linear.app/a8c/issue/PCDROID-724/podcast-details-analytics
Testing Instructions
./gradlew :tv:installDebug.episode_actions_shownwith the matchingsourceinadb logcat.episode_detail_shownwith the samesource.Screenshots or Screencast
Checklist
./gradlew spotlessApply)