Skip to content

[TV] Playlist analytics - #5738

Open
sztomek wants to merge 4 commits into
feat/tv-analytics-your-podcastsfrom
feat/tv-analytics-playlists
Open

[TV] Playlist analytics#5738
sztomek wants to merge 4 commits into
feat/tv-analytics-your-podcastsfrom
feat/tv-analytics-playlists

Conversation

@sztomek

@sztomek sztomek commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Description

Third screen of the Android TV → Apple TV analytics parity program (EventHorizon only): the Playlists tab (list + detail). Stacked on the Your Podcasts analytics PR. Verified against the tvOS app across every tracking layer — all direct Analytics.track, no hidden helper layer.

Event EventHorizon class Properties Fires when
filter_list_shown FilterListShownEvent filter_count playlists list shown (once, after load)
filter_create_button_tapped FilterCreateButtonTappedEvent the create button (opens the "download app" modal — you can't create playlists on TV)
filter_shown FilterShownEvent filter_type a playlist's detail is shown
filter_show_archived_tapped / filter_hide_archived_tapped data objects toggle the archived filter (by the new value)
filter_play_all_tapped FilterPlayAllTappedEvent filter_type Play All (only when the playlist has episodes)
filter_play_all_replace_and_play_tapped FilterPlayAllReplaceAndPlayTappedEvent filter_type, save_up_next the replace-Up-Next confirmation (save vs discard)
filter_play_all_dismissed FilterPlayAllDismissedEvent filter_type the replace-Up-Next confirmation is cancelled

filter_type = manual / smart, derived from Playlist.Type.analyticsValue (→ PlaylistType), matching iOS's isManual ? "manual" : "smart".

Archive toggle is manual-only for analytics, matching iOS (PlaylistDetailView.swift:225 gates the archive menu behind if model.isManual) — so filter_show/hide_archived_tapped fires only for manual playlists.

Archive toggle is now manual-only in the UI too. A review surfaced that the Android TV archive-filter toggle was shown for smart playlists as well, whereas Apple TV only shows it for manual (PlaylistDetailView.swift:225). A second commit hides the toggle for smart playlists (with the Play-All left-focus preserved on the sort button), closing both the analytics and the feature gap.

Faithful to tvOS: show-vs-hide-archived is chosen by the new value; filter_play_all_tapped only fires when there are episodes (mirrors iOS's guard !episodes.isEmpty); the replace/dismiss events carry the same save_up_next / filter_type payloads. All wired into the existing VM methods (toggleArchiveFilter, playAll, replaceUpNextAndPlay) plus CallOnce for filter_shown and a dismiss hook on the confirmation modal.

Testing Instructions

  1. ./gradlew :tv:installDebug; open the Playlists tab → confirm one filter_list_shown with filter_count in adb logcat.
  2. Tap the create button → filter_create_button_tapped (+ the download-app modal).
  3. Open a playlist → filter_shown with filter_type. Toggle the archived filter → filter_show_archived_tapped / filter_hide_archived_tapped.
  4. Play All on a non-empty playlist → filter_play_all_tapped; if it prompts to replace Up Next: save → filter_play_all_replace_and_play_tapped save_up_next=true; without saving → save_up_next=false; cancel → filter_play_all_dismissed.

Screenshots or Screencast

SCR-20260813-oxgz

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 — existing events, no schema change needed

@sztomek sztomek changed the title Mirror the Apple TV Playlists screen analytics [TV] Playlist analytics Aug 13, 2026
@sztomek sztomek added this to the 8.19 milestone Aug 13, 2026
@sztomek
sztomek marked this pull request as ready for review August 13, 2026 14:58
@sztomek
sztomek requested a review from a team as a code owner August 13, 2026 14:58
@sztomek
sztomek requested review from geekygecko and removed request for a team August 13, 2026 14:58
@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 3m 39s —— View job


Review

  • Gather context (diff vs feat/tv-analytics-your-podcasts)
  • Review the analytics wiring (both view models)
  • Review the Compose changes (archive toggle gating, focus, CallOnce)
  • Review the tests
  • Post review

Overall this looks solid and consistent with the Your Podcasts PR it stacks on — the xxxTracked flag + LaunchedEffect(uiState) pattern matches TvYourPodcastsScreen.kt:58-76, CallOnce is already the convention for "shown" events (TvHomeScreen.kt:68), and the events all read straight off the assisted playlistType so nothing depends on load order. Tests cover every new tracking method. A few things worth addressing, one of which is a real (small) user-facing regression from the second commit.


1. Smart playlists can get stuck showing archived episodes (behavioural, from commit 07d543e)

isShowingArchivedFlow is seeded from a persisted per-playlist preference:

// TvPlaylistDetailsViewModel.kt:74
private val isShowingArchivedFlow = MutableStateFlow(preferences.isPlaylistShowingArchived(playlistUuid))

and TvPreferences.kt:16-22 stores it in SharedPreferences keyed by uuid. Before this PR the archive toggle was shown for smart playlists too, so a user could have flipped it on. After the toggle is hidden (TvPlaylistDetailsScreen.kt:236), that saved true still filters into uiState, and the UI no longer offers any way to turn it back off — the smart playlist permanently shows archived episodes.

Suggested fix — ignore the pref for the type that can't express it:

private val isShowingArchivedFlow = MutableStateFlow(
    playlistType == Playlist.Type.Manual && preferences.isPlaylistShowingArchived(playlistUuid),
)

Fix this →

2. toggleArchiveFilter() on a smart playlist mutates state but deliberately doesn't track

// TvPlaylistDetailsViewModel.kt:115-122
fun toggleArchiveFilter() {
    val isShowingArchived = !isShowingArchivedFlow.value
    if (playlistType == Playlist.Type.Manual) {
        eventHorizon.track(...)
    }
    preferences.setPlaylistShowingArchived(playlistUuid, isShowingArchived)
    isShowingArchivedFlow.value = isShowingArchived
}

This is the worst of both worlds if it's ever reachable for smart: state and the persisted pref change, but no event is emitted — an untracked behaviour change. Since the UI now gates the control on isManual, I'd make the whole method a no-op for smart (early return) rather than only the tracking. Then the analytics guard falls out for free, the existing toggling the archive filter on a smart playlist tracks nothing test still passes, and issue #1 can't recur from the toggle path either. If you prefer to keep it defensive as-is, a short comment explaining that the guard mirrors PlaylistDetailView.swift:225 would help the next reader.

3. Sort button left-focus is now plumbed through modifier rather than a parameter

// TvPlaylistDetailsScreen.kt:249-253
modifier = if (isManual) Modifier else Modifier.focusProperties { leftFocusRequester?.let { left = it } }

TvSortButton applies modifier to the wrapping Box, not to the focusable IconButton (TvEpisodeListControls.kt:98-104). This relies on focus properties being inherited by descendant focus targets, which does normally work, but it's asymmetric with TvArchivedFilterButton, which takes an explicit leftFocusRequester: FocusRequester? and applies it directly to its Button (TvEpisodeListControls.kt:33, 41-45). I'd mirror that here — add leftFocusRequester: FocusRequester? = null to TvSortButton and apply it on the IconButton — so both controls read the same way and the behaviour doesn't depend on inheritance through the Box. Either way, worth explicitly confirming D-pad-left from the sort button lands on Play All for a smart playlist on a device, since that's the path the second commit changed.

4. Test gap: the "no episodes ⇒ no filter_play_all_tapped" guard is untested

playAll() (TvPlaylistDetailsViewModel.kt:128-131) has the explicit iOS-parity guard (guard !episodes.isEmpty), and it's the one piece of conditional tracking logic in playAll — but the tests only cover the non-empty case. A negative test (emit a playlist with no episodes, call playAll(), verify(eventHorizon, never()).track(any<FilterPlayAllTappedEvent>())) would lock that in. Same for replaceUpNextAndPlay(saveUpNext = false), which is currently only exercised with true.

Add these tests →

Nits

  • filterListShownTracked should probably be rememberSaveable (TvPlaylistsScreen.kt:78). The sibling state on that screen uses rememberSaveable, and CallOnce itself is rememberSaveable-backed (ComposeUtils.kt:17), so as written an activity recreation re-fires filter_list_shown. Same shape as the base PR, so fine to leave for consistency — but worth fixing in both if you care about duplicate counts.
  • PR description is missing the third commit's events. filter_sort_by_tapped / filter_sort_by_changed (with sort_order) aren't in the table; worth adding so the description matches what ships.
  • onSortTap: () -> Unit = {} on private composables (TvPlaylistDetailsScreen.kt:162, 215). Defaults on private composables let a wiring gap pass silently; since there's exactly one call site each, making it a required param (before modifier, alongside onChangeSortType) is a bit safer. The previews would just pass {}.
  • filter_shown fires before the playlist resolves, so a stale/deleted playlist that immediately hits NotFound → onClose() still emits one. That matches an onAppear-style event so it's probably intended — just noting it as expected noise in the data.

I reviewed statically and did not run a build or the unit tests here; CI covers those.
· Branch: feat/tv-analytics-playlists

@sztomek
sztomek force-pushed the feat/tv-analytics-your-podcasts branch from 3061664 to 0fc0af0 Compare August 14, 2026 12:19
@sztomek
sztomek force-pushed the feat/tv-analytics-playlists branch from 77c6a7b to 0bbc85f Compare August 14, 2026 12:19
@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

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


Review

  • Gather context (diff vs feat/tv-analytics-your-podcasts)
  • Check whether previous review points were addressed
  • Re-review analytics wiring, Compose changes, tests
  • Post review

Note on this run: the trigger was a synchronize, but the PR diff is byte-identical to what I reviewed yesterday — the three commits were only rebased (7d56105, 2154ff6, 0bbc85f, all timestamped before the previous review). So none of the earlier points have been addressed yet; they're restated below with a couple of additions from a second pass.


1. Smart playlists can get permanently stuck showing archived episodes ⚠️ (still open — the one real user-facing bug)

isShowingArchivedFlow is seeded from a persisted per-playlist preference, with no gate on type:

// TvPlaylistDetailsViewModel.kt:74
private val isShowingArchivedFlow = MutableStateFlow(preferences.isPlaylistShowingArchived(playlistUuid))

TvPreferences.isPlaylistShowingArchived is a plain SharedPreferences boolean keyed by uuid (show_archived_playlist_$uuid), and I confirmed nothing else ever resets it. Before this PR the archive toggle was rendered for smart playlists too, so a user could have flipped it on; after TvPlaylistDetailsScreen.kt:236 gates the button behind isManual, that saved true keeps filtering into uiState and there is no longer any UI to turn it off. That smart playlist shows archived episodes forever.

private val isShowingArchivedFlow = MutableStateFlow(
    playlistType == Playlist.Type.Manual && preferences.isPlaylistShowingArchived(playlistUuid),
)

Fix this →

2. toggleArchiveFilter() on a smart playlist mutates state but deliberately doesn't track (still open)

// TvPlaylistDetailsViewModel.kt:115-122
fun toggleArchiveFilter() {
    val isShowingArchived = !isShowingArchivedFlow.value
    if (playlistType == Playlist.Type.Manual) {
        eventHorizon.track(...)
    }
    preferences.setPlaylistShowingArchived(playlistUuid, isShowingArchived)
    isShowingArchivedFlow.value = isShowingArchived
}

If this is ever reachable for smart, it's the worst of both worlds: state and the persisted pref change, but no event is emitted — an untracked behaviour change. Since the UI now gates the control on isManual, an early return for smart is cleaner than guarding only the tracking: the analytics guard falls out for free, the existing toggling the archive filter on a smart playlist tracks nothing test still passes, and issue #1 can't be re-introduced through this path. If you'd rather keep it defensive, a one-line comment pointing at PlaylistDetailView.swift:225 would help the next reader.

3. Sort-button left-focus is plumbed through modifier instead of a parameter (still open)

// TvPlaylistDetailsScreen.kt:249-253
modifier = if (isManual) Modifier else Modifier.focusProperties { leftFocusRequester?.let { left = it } }

TvSortButton applies modifier to the wrapping Box, not the focusable IconButton (TvEpisodeListControls.kt:98-104), so this depends on focus properties being inherited by a descendant focus target. That normally works, but it's asymmetric with TvArchivedFilterButton, which takes an explicit leftFocusRequester: FocusRequester? and applies it directly to its Button (TvEpisodeListControls.kt:33, 41-45). Mirroring that — leftFocusRequester: FocusRequester? = null on TvSortButton, applied on the IconButton — makes both controls read the same and removes the inheritance assumption. Either way, please confirm on a device that D-pad-left from the sort button lands on Play All for a smart playlist; that's exactly the path commit 2154ff6 changed.

4. Test gap: the "no episodes ⇒ no filter_play_all_tapped" guard is untested (still open)

playAll() (TvPlaylistDetailsViewModel.kt:128-131) carries the iOS-parity guard (guard !episodes.isEmpty) and it's the only conditional tracking in playAll, but tests only cover the non-empty case. A negative test (verify(eventHorizon, never()).track(any<FilterPlayAllTappedEvent>()) after playAll() on an empty playlist) locks it in. Same for replaceUpNextAndPlay(saveUpNext = false), currently only exercised with true.

Add these tests →


Verified as correct on this pass

  • No duplicate filter_sort_by_changed / archive events. TvSortButton only calls onSelect when option != selected (TvEpisodeListControls.kt:123), and TvArchivedFilterButton only calls onToggleArchiveFilter when the value actually flips (:67, :77) — so changeSortType's unconditional track can't fire on a no-op selection, and toggleArchiveFilter's "new value" derivation is sound.
  • filter_list_shown cadence matches iOS onAppear. TvScaffold.kt:91-101 renders tabs through a when (tab), so the screen is disposed on tab switch and the event re-fires on each entry — the right semantics, not a leak.
  • sort_order payload comes from PlaylistEpisodeSortType.analyticsValue → EventHorizonSortType and filter_type from Playlist.Type.analyticsValue → PlaylistType; both read off the assisted playlistType, so nothing depends on load order.
  • playAll() now snapshots episodes before viewModelScope.launch instead of inside it — no behaviour change (viewModelScope is Main.immediate), just noting the move is intentional and safe.

Nits

  • filterListShownTracked could be rememberSaveable (TvPlaylistsScreen.kt:78). Sibling state on that screen uses rememberSaveable, and CallOnce is itself rememberSaveable-backed (ComposeUtils.kt:17), so as written an activity recreation re-fires filter_list_shown. Given the tab is disposed on switch anyway, this only affects config-change/process-recreation duplicates. Same shape as the base PR — fine to leave, but worth fixing in both if duplicate counts matter.
  • TvSortButton.onExpand defaults to {}, so the podcast details screen (TvPodcastDetailsScreen.kt:294) silently tracks nothing on sort tap. Presumably correct if tvOS has no equivalent podcast event — just flagging that the default makes the omission invisible.
  • PR description is missing the third commit's events. filter_sort_by_tapped / filter_sort_by_changed (with sort_order) aren't in the table.
  • onSortTap: () -> Unit = {} on private composables (TvPlaylistDetailsScreen.kt:162, 215) — with exactly one call site each, a required param (next to onChangeSortType) prevents a silent wiring gap; previews would pass {}.
  • filter_shown fires before the playlist resolves, so a stale/deleted playlist that immediately hits NotFound → onClose() still emits one. Matches onAppear semantics — noting it as expected noise in the data.

I reviewed statically and did not run a build or the unit tests here; CI covers those.
· Branch: feat/tv-analytics-playlists

@sztomek
sztomek force-pushed the feat/tv-analytics-your-podcasts branch from 0fc0af0 to 3018745 Compare August 14, 2026 12:46
@sztomek
sztomek force-pushed the feat/tv-analytics-playlists branch from 0bbc85f to f82412c Compare August 14, 2026 12:50
@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

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


Review

  • Gather context (diff vs feat/tv-analytics-your-podcasts)
  • Check the new "Address PR comments" commit (f82412c) against previous review points
  • Re-review analytics wiring, Compose changes, tests
  • Post review

f82412c addresses points 1, 2 and 3 from the previous rounds, all correctly. Point 4 (the test gap) is still open. Details below.


Previous points — status

✅ 1. Smart playlists stuck showing archived episodes — fixed

// TvPlaylistDetailsViewModel.kt:74-76
private val isShowingArchivedFlow = MutableStateFlow(
    playlistType == Playlist.Type.Manual && preferences.isPlaylistShowingArchived(playlistUuid),
)

Correct: a previously-persisted true on a smart playlist is now ignored, so the hidden toggle can't strand anyone. The stale show_archived_playlist_<uuid> boolean stays in SharedPreferences, but since it's never read for smart playlists and a playlist can't change type, no cleanup is needed. Covered by the new smart playlists ignore the stored archived preference test.

✅ 2. toggleArchiveFilter() early-returns for smart — fixed

// TvPlaylistDetailsViewModel.kt:118-124
fun toggleArchiveFilter() {
    if (playlistType != Playlist.Type.Manual) return
    ...
}

State, the persisted pref and the event now all move together — no untracked mutation path, and the analytics guard falls out for free. The existing toggling the archive filter on a smart playlist tracks nothing test still passes.

✅ 3. Sort-button left-focus is now a real parameter — fixed

TvSortButton takes leftFocusRequester: FocusRequester? = null and applies it to the focusable IconButton (TvEpisodeListControls.kt:95, 106), and the call site is now leftFocusRequester = if (isManual) null else leftFocusRequester (TvPlaylistDetailsScreen.kt:248). No more reliance on focus-property inheritance through the wrapping Box, and it reads the same as TvArchivedFilterButton. Still worth a quick device check that D-pad-left from the sort button lands on Play All for a smart playlist, since that's the path 573b0c3 changed.

Minor: TvArchivedFilterButton uses an if (leftFocusRequester != null) { Modifier.focusProperties { … } } else { Modifier } while TvSortButton unconditionally applies Modifier.focusProperties { leftFocusRequester?.let { left = it } }. Both are functionally identical (an empty focusProperties block is a no-op); purely a style inconsistency in the same file if you want them to match.

⚠️ 4. Test gap — still open

playAll() (TvPlaylistDetailsViewModel.kt:129-132) still has the iOS-parity guard as the only conditional tracking in the method, and it's still only exercised with a non-empty playlist (play all tracks the tapped event, test line 403). Likewise replaceUpNextAndPlay is only tracked-asserted with saveUpNext = true (line 420), so a payload regression on the false branch wouldn't be caught.

Add these tests →


Notes on the new test

smart playlists ignore the stored archived preference (test line 103) is meaningful — with the pref stubbed to true it would fail without the type gate. Two small things:

  • It emits a ManualPlaylist (via the shared playlist(...) helper) through smartPlaylistFlow. It compiles and passes because the VM only consumes it as Playlist, but a test named "smart playlists…" asserting on a manual fixture is a trap for the next person — a SmartPlaylist fixture (or a comment) would read better.
  • Now that toggleArchiveFilter early-returns, the smart-playlist toggle test could also assert the no-op on state (isShowingArchivedOnDevice stays false and verify(prefs, never()).setPlaylistShowingArchived(any(), any())), not just the absence of events. Cheap, and it locks in the fix for Update issue templates #1 from the second direction.

Remaining nits (unchanged, all optional)

  • filterListShownTracked could be rememberSaveable (TvPlaylistsScreen.kt:78) — as written an activity recreation re-fires filter_list_shown. Same shape as the base PR; fine to leave, worth fixing in both if duplicate counts matter.
  • onSortTap: () -> Unit = {} on private composables (TvPlaylistDetailsScreen.kt:161, 214) — one call site each, so a required param prevents a silent wiring gap.
  • PR description is missing the third commit's eventsfilter_sort_by_tapped / filter_sort_by_changed (with sort_order) aren't in the table.
  • filter_shown fires before the playlist resolves, so a deleted playlist that goes straight to NotFound → onClose() still emits one. Matches onAppear semantics — noting it as expected noise.

Nothing blocking beyond the test gap. I reviewed statically and did not run a build or the unit tests here; CI covers those.
· Branch: feat/tv-analytics-playlists

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