[TV] Add onboarding analytics - #5773
Conversation
|
Claude finished @sztomek's task in 4m 51s —— View job Code review: TV onboarding analytics
SummaryClean, well-scoped PR. The implementation follows the pattern already established in this repo ( No correctness bugs found. No security or performance concerns: every event is either param-free or enum-valued, so there's no PII or user-supplied data in the payloads, and tracking is synchronous fire-and-forget off the existing The decisions documented in the PR description all hold up against the code:
Inline comments
Two additional notes (no action required)
Verification I could not doThe --- • branch |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
sztomek
changed the title
sztomek
added
[Type] Feature
|
Claude finished @sztomek's task in 3m 8s —— View job Code review: TV onboarding analytics (re-review at
|
| # | Was | Now |
|---|---|---|
| 1 | flow = OnboardingFlowType.Unknown on TV events |
Accepted as-is. No // comment added, but the PR description now documents the reasoning. Fine — see the new finding below, which is the mobile half of the same question. |
| 2 | trackXxxClicked naming |
✅ Fixed — trackSignInTapped / trackCreateAccountTapped / trackBrowseNoAccountTapped |
| 3 | Create Account → Sign In fires no setup_account_button_tapped |
✅ Documented — the undercount caveat is now spelled out in the PR description. Deliberate, not an omission. |
| 4 | TvSignInViewModelTest needs a createViewModel() helper |
✅ Fixed (TvSignInViewModelTest.kt:157). The "why are these stubs here" comment wasn't added — harmless. |
New finding this round
The 2026-08-25 catalog bump makes flow a required property on UserAccountCreatedEvent / CreateAccountNextButtonTappedEvent, and this PR satisfies that by hardcoding OnboardingFlowType.Unknown at all four new call sites. For the two TV sites and CreateEmailFragment that's correct — there is genuinely no flow in scope.
For the two in SyncManagerImpl it isn't: the callers do have the flow. OnboardingCreateAccountViewModel takes flow: OnboardingFlow on every other tracking method it exposes, and GoogleSignInButtonViewModel.startGoogleOneTapSignIn(flow, …) already uses flow.analyticsValue immediately before calling into loginWithGoogle. The result is that on mobile, the whole account-creation funnel carries a real flow except the terminal conversion event, which is now permanently unknown — so flow-segmented funnels break exactly at the conversion step. Details and a suggested fix are in the inline comment on SyncManagerImpl.kt:586; a follow-up issue would be an acceptable resolution if you want to keep this PR TV-scoped.
qr_code identity — traced, no bug
I walked the persistence paths for the new LoginIdentity.QrCode and everything holds (written up inline on LoginIdentity.kt:21):
- Downgrade-safe — an older build's
valueOf("QrCode")returnsnull, butgetLoginIdentity()falls back toPocketCasts(SyncAccountManagerImpl.kt:86), so it degrades rather than crashes. - Token refresh unaffected —
downloadTokensswitches ongetSignInType()(SIGN_IN_TYPE_KEY, alwaysTokens), never onLoginIdentity. Description is accurate. isGoogleLogin()is the only behavioural consumer and is unchanged (QrCode != Google, asPocketCasts != Googlewas).- One theoretical, currently unreachable gap:
LoginIdentity.Adapter.fromJsonfeeds a non-nullWatchSyncAuthData.loginIdentity, so an older watch build receiving"QrCode"would throw rather than degrade. Device auth is TV-only (TvDeviceAuth.kt:36is the sole caller) and TV has no watch sync — only relevant if device auth ever ships on phone.
Also worth a heads-up to dashboard owners: TV device-auth sign-ins move from source = password to source = qr_code, so saved segments filtering on password will show a step change at this release.
Everything else
The TV analytics work itself is clean and I found no correctness bugs. The TvWelcomeContent / TvCreateAccountContent extraction keeps previews Hilt-free, tracking fires before navigation so nothing is lost to the transition, and the new tests cover each trackXxx method. No security or performance concerns — every payload is param-free or enum-valued, and tracking is fire-and-forget off the existing singleton.
Two carry-over notes from last round, still true and still no action needed: CallOnce uses rememberSaveable, so *_shown won't re-fire on back-navigation the way a tvOS onAppear would (matches mobile, but the two platforms' *_shown counts aren't strictly comparable); and setup_account_dismissed / create_account_dismissed have no TV equivalent, which is the natural next step if you want drop-off.
Verification I could not do
The tv module still wasn't compiled here — no Gradle cache for com.automattic:eventhorizon:pocket-casts-2026-08-25_18-30-35, so I can't confirm OnboardingFlowType.Unknown, LoginIdentityType.QrCode, or the new required flow parameters exist as used. GitHub Actions on this PR shows only Validate Gradle Wrapper and the Danger trigger; the actual build runs on Buildkite. Please confirm ./gradlew :tv:testDebugUnitTest, the :modules:services:repositories / :servers / :features:account unit tests, and spotlessCheck are green there before merge.
--- • branch feat/tv-onboarding-analytics
Project dependencies changeslist! Upgraded Dependencies
com.automattic:eventhorizon:pocket-casts-2026-08-27_07-40-16, (changed from pocket-casts-2026-08-13_07-58-34)tree +--- project ':modules:features:account'
| \--- project ':modules:features:search'
| \--- project ':modules:services:analytics'
-| +--- com.automattic:eventhorizon:pocket-casts-2026-08-13_07-58-34
+| +--- com.automattic:eventhorizon:pocket-casts-2026-08-27_07-40-16
| +--- project ':modules:services:model'
-| | +--- com.automattic:eventhorizon:pocket-casts-2026-08-13_07-58-34 (*)
+| | +--- com.automattic:eventhorizon:pocket-casts-2026-08-27_07-40-16 (*)
| | \--- project ':modules:services:utils'
| | \--- project ':modules:services:payment'
-| | \--- com.automattic:eventhorizon:pocket-casts-2026-08-13_07-58-34 (*)
+| | \--- com.automattic:eventhorizon:pocket-casts-2026-08-27_07-40-16 (*)
| \--- project ':modules:services:preferences'
-| \--- com.automattic:eventhorizon:pocket-casts-2026-08-13_07-58-34 (*)
+| \--- com.automattic:eventhorizon:pocket-casts-2026-08-27_07-40-16 (*)
\--- 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-08-13_07-58-34 (*)
+ \--- com.automattic:eventhorizon:pocket-casts-2026-08-27_07-40-16 (*) |
|
Claude finished @sztomek's task in 2m 34s —— View job Code review: TV onboarding analytics (re-review after
|
| Round | Finding | Status |
|---|---|---|
| 1 | flow = Unknown on TV events |
✅ Fixed in 60c791a → InitialOnboarding |
| 1 | trackXxxClicked naming |
✅ Fixed |
| 1 | Create Account → Sign In fires no setup_account_button_tapped |
✅ Documented as deliberate |
| 1 | TvSignInViewModelTest needs a createViewModel() helper |
✅ Fixed |
| 2 | SyncManagerImpl hardcodes flow = Unknown on user_account_created |
⏳ Still open — unchanged at SyncManagerImpl.kt:586 and :632. Mobile's whole account-creation funnel carries a real flow except the terminal conversion event. Follow-up is acceptable; leaving it silently isn't, since unknown on a required property is sticky once dashboards exist. |
| 2 | LoginIdentity.QrCode persistence trace |
✅ No bug — reasoning recorded inline |
Everything else
Re-read the full diff; nothing else changed since round 2 and I found no new correctness bugs. Tracking still fires before navigation, the TvWelcomeContent / TvCreateAccountContent split keeps previews Hilt-free, every new event is param-free or enum-valued (no PII), and tracking is fire-and-forget off the existing EventHorizon singleton — no security or performance concerns.
Carry-over notes, still true, still no action needed: CallOnce uses rememberSaveable so *_shown won't re-fire on back-navigation the way a tvOS onAppear would; and setup_account_dismissed / create_account_dismissed have no TV equivalent.
Verification I could not do
The tv module still wasn't compiled here — no Gradle cache for com.automattic:eventhorizon:pocket-casts-2026-08-25_18-30-35, so I can't confirm LoginIdentityType.QrCode or the new required flow parameters exist as used (OnboardingFlowType.InitialOnboarding I can confirm, via the pre-existing OnboardingFlow.kt:23 reference). GitHub Actions on this PR shows only Validate Gradle Wrapper (passing) and this review job; the real build runs on Buildkite. Please confirm ./gradlew :tv:testDebugUnitTest, the :modules:services:repositories / :servers / :features:account unit tests, and spotlessCheck are green there before merge.
• branch feat/tv-onboarding-analytics
|
Claude finished @sztomek's task in 1m 35s —— View job Code review: incremental (since
|
…uth sign-in source
9c743d2 to
1fb257a
Compare
|
Claude finished @sztomek's task in 41s —— View job Code review: incremental (since the last round)Since my last review the branch adds Still open from earlier rounds
|
3 participants
Description
Ports the Apple TV onboarding analytics to Android TV. The Android TV onboarding flow (Welcome, Sign In, Create Account, Syncing) previously fired no analytics; this brings it to parity with tvOS so both platforms report the same onboarding funnel.
Events added:
setup_account_shownflowsetup_account_button_tappedflow,button = sign_insetup_account_button_tappedflow,button = create_accountbrowse_no_account_tappedsign_in_showncreate_account_shownflowsign_in_sync_shownImplementation mirrors the existing merged TV analytics pattern (
EventHorizoninjected into a@HiltViewModelexposingtrackXxx()methods, invoked from the screen viaCallOnce { }).The catalog bump also required adapting two unrelated call sites to a newly-required
flowproperty (user_account_created,create_account_next_button_tapped); both passOnboardingFlowType.Unknownas that layer has no onboarding flow in scope.The pin is
pocket-casts-2026-08-27_07-40-16rather than the08-25build so it also carries the newSearchResultFilterType.Foldersvalue (EventHorizonSchemas#125). Going08-25→08-27adds no further required-property breakage — only the enum value.With #5769 (TV Search analytics) now merged, this branch also flips the TV Search folder filter chip from the placeholder
SearchResultFilterType.Unknownto the realSearchResultFilterType.Folders(TvSearchViewModel), so focusing the Folders chip reportsfilter = foldersinstead ofunknown— closing the blocking finding from that thread. Covered by a newTvSearchViewModelTestcase.Decisions / notes
flow = OnboardingFlowType.Unknownon the onboarding screen events. tvOS sends no flow, but the Android generated events require a non-nullOnboardingFlowType.sign_in_shownfires when the Sign In screen is shown (not on the Welcome "Sign In" tap), which covers both entry points into sign-in: the Welcome screen and the browse-without-account Home.user_signed_in/user_signin_failedare emitted centrally bySyncManagerImpl; duplicating them from the screens would double-count.sign_in_type_tappedis omitted — Android TV sign-in is QR-only (no QR/password picker).create_account_shown— Android TV "create account" is a QR to the web/createpage, and the device-auth path signs in an existing account (isNewAccount = false), souser_account_createddoes not apply on TV.setup_account_button_tappedfires only from the Welcome screen (matching tvOS, where it is Welcome-only). The Create Account screen's "Sign In" button and the browse-without-account profile-modal entry points navigate without firing it, sosetup_account_button_tappedundercounts relative tosign_in_shown/create_account_shown— i.e. "tapped → shown" is not a clean drop-off rate for these events.Fixes PCDROID-731 https://linear.app/a8c/issue/PCDROID-731/onboarding-analytics
Testing Instructions
tvapp on an Android TV device/emulator, signed out.setup_account_shownfires.setup_account_button_tapped(button = sign_in), thensign_in_shownon the Sign In screen.setup_account_button_tapped(button = create_account) andcreate_account_shown.browse_no_account_tapped.sign_in_sync_shownon the Syncing screen, and thatuser_signed_infires exactly once (fromSyncManagerImpl) withsource = qr_codeand no duplicate.Unit tests:
./gradlew :tv:testDebugUnitTest(newTvWelcomeViewModelTest,TvCreateAccountViewModelTest; updatedTvSignInViewModelTest,TvSyncingViewModelTest). Catalog bump verified by compiling:app,:tv,:automotive,:wearand running:modules:services:repositories,:modules:services:servers,:modules:features:accountunit tests.Screenshots or Screencast
No UI changes — analytics only.
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xml