Skip to content

[TV] Actively poll API in create account flow - #5784

Merged
sztomek merged 4 commits into
mainfrom
feat/tv-create-account-device-auth
Aug 27, 2026
Merged

sztomek merged 4 commits into
mainfrom
feat/tv-create-account-device-auth

Conversation

@sztomek

@sztomek sztomek commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

Description

Brings the Android TV Create Account screen to iOS tvOS parity. Previously it was a degraded two-step flow: a static /create web QR, a "come back and sign in" message, and a Sign in button that dumped you into the separate device-auth screen. Now it runs the device-auth pairing flow directly (the same one the Sign In screen uses) and auto-advances to Syncing on success — one shot, no manual hand-off.

It also lands the deferred user_account_created analytics event (the last of the three events split out of #5773).

How it works

  • The create screen now runs deviceAuthFlow(syncManager, isNewAccount = true) and renders Loading / Ready (QR + steps + code digits) / Error / Complete, mirroring TvSignInScreen, with create-framed copy ("Create your free account" + "Create your account or log in"). On Complete it navigates to SYNCING.
  • Analytics: loginWithDeviceAuth gained an explicit isNewAccount: Boolean (no default), plumbed into AuthResultModel (was hardcoded false; DeviceTokenResponse has no isNew field). The create screen passes true, so the existing trackSignIn fires user_account_created (source = qr_code) instead of user_signed_in. This is screen-attributed, exactly like iOS (both tvOS screens use the same pairing session and attribute the event by which screen you're on). create_account_shown still fires.
  • The follow-podcast "Create your account or log in" modal (TvPodcastDetailsViewModel) passes isNewAccount = false. It's a mixed create/login surface — its own copy says "Create your account or log in", and existing users following a podcast are a normal path — so attributing every follow as a creation would inflate user_account_created. It emits user_signed_in instead, deliberately undercounting creations rather than inflating them.
  • flow on the created event: user_account_created is emitted through trackSignIn, which hardcodes flow = unknown (platform-wide — password signup via trackRegister does the same). So although create_account_shown carries flow = initial_onboarding, the paired user_account_created lands with flow = unknown, and the two can't be joined on flow. Left as-is here to avoid changing shared mobile sign-in behavior.

QR parity note

The create QR is the device-auth pairing URL (pocketcasts.net/pair on debug/staging, .com on release) — the same QR the Sign In screen shows. Verified against iOS: both CreateAccountView and SignInView encode pairing.pairURLComplete (the server's verificationUriComplete); the screens differ only in copy. The old distinct static /create QR was the non-parity bit removed here.

Caveat (iOS-parity imprecision)

Because the server's device-token response carries no is-new-account signal, user_account_created is attributed by screen — a user who signs into an existing account from the dedicated create screen is still counted as a creation (iOS has the identical behavior). The follow-podcast modal, being a mixed create/login surface, deliberately attributes as sign-in instead. The paired Sync-onboarding-notification write is benign: that notification only schedules for signed-out users, so it's suppressed post-sign-in regardless.

Stacking

#5773 (feat/tv-onboarding-analytics), which provided create_account_shown and the qr_code LoginIdentity, has now merged. This branch is rebased onto main and contains only the device-auth create-account commits.

Testing Instructions

  1. Signed out, on a debug build (staging): Welcome → Create free account.
  2. You get "Create your free account" with a live QR, steps ("Scan… / Create your account or log in / confirm code"), and 6 user-code chips.
  3. Scan the QR (or open pocketcasts.net/pair and enter the code) on your phone; create a new account (or log in) and confirm the code.
  4. The TV auto-advances Create account → Syncing → Home — no "come back and sign in" step.
  5. Error path (no network): "Something went wrong" + Retry.
  6. Analytics: create_account_shown on appear; on success user_account_created (qr_code) (vs user_signed_in (qr_code) from the Sign In screen).

Note: debug points at staging, so pair against pocketcasts.net; use a debugProd build for production (pocketcasts.com).

Screenshots or Screencast

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • 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 modules/services/localization/src/main/res/values/strings.xml
  • 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. (user_account_created / create_account_shown already exist in the schema; no new events added.)

I have tested any UI changes...

  • with different themes (TV is single dark theme)
  • with a landscape orientation (TV is landscape-only)
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@claude

claude Bot commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

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


Review: TV create account via device auth

  • Read the PR diff against feat/tv-onboarding-analytics
  • Review device-auth plumbing (SyncManager / SyncManagerImpl)
  • Review TV create-account screen + ViewModel
  • Review tests & string removals
  • Post review

The change itself is well-shaped: reusing deviceAuthFlow + TvSignInQrContent is clearly the right call, the state handling mirrors TvSignInScreen (including rememberUpdatedState around the completion callback, which is easy to get wrong), and navigateClearingBackStack(SYNCING) matches what the sign-in route already does so the user can't back into a stale pairing screen. Previews and tests were added for the new states. I found no correctness bug in the flow.

Five things worth acting on, posted as inline comments:

# Where Issue
1 values/strings.xml Orphaned translations may trip the ExtraTranslation lint check
2 TvPodcastDetailsViewModel.kt:103 Attribution inflation is worse in the follow modal than on the create screen
3 SyncManager.kt:71 Caller-asserted flag lands in a field that is server truth everywhere else
4 TvCreateAccountScreen.kt:108 ~150 lines duplicated verbatim from TvSignInScreen.kt
5 TvCreateAccountViewModelTest.kt:54 Test can pass without ever observing Ready

1. Orphaned translations → possible lint failure

You removed tv_create_account_subtitle and tv_create_account_come_back from the default locale, but they're still declared in values-de, values-es, values-fr-rCA, values-nl, values-ar, … . MissingTranslation is ignored in the root lint.xml; ExtraTranslation is not, and app/build.gradle.kts:59 has checkDependencies = true, so the app's lint run sees the localization module. Worth a ./gradlew :app:lintRelease before merge. (Flagging, not asserting — I couldn't run Gradle here.)

2. The follow modal is the riskier attribution surface

The PR description frames the screen-attribution imprecision as create-screen-only, but isNewAccount = true also went onto TvPodcastDetailsViewModel.startAccountAuth(). That modal's own copy says "Create your account or log in", and it's shown to any signed-out user who tries to follow a podcast — existing users logging in there is a normal path, and each one now emits user_account_created plus flips updateUserFeatureInteraction(OnboardingNotificationType.Sync). Your "notification write is benign" analysis holds (OnboardingNotificationType.Sync only schedules for signed-out users), but the event inflation may not be. Server-side isNew in DeviceTokenResponse is the real fix; failing that, consider leaving this call site at false, and please record the tradeoff in a code comment rather than only in the PR body.

3. isNewAccount semantics

AuthResultModel.isNewAccount is response.isNew everywhere else (SyncManagerImpl.kt:743) and drives behaviour, not just analytics (OnboardingFlowRoutes.kt:139/202/240 route the paywall on it). For device auth it now means "the caller thought this was a signup". No current consumer is affected — both TV call sites discard the result — but a KDoc line and/or a name like reportAsNewAccount would stop a future reader trusting it as fact. Same comment covers the = false default, which lets new call sites pick sign-in attribution by accident.

4. Duplication with TvSignInScreen.kt

TvCreateAccountError is byte-identical to TvSignInError; TvCreateAccountLoading, TvCreateAccountContent and createAccountSteps differ from their sign-in twins by exactly one string each; TvCreateAccountViewModel's polling/retry block is a line-for-line copy of TvSignInViewModel's. Suggested a parameterised TvDeviceAuthScreen(state, title, middleStep, onRetry) in the signin package plus a shared state holder. Not a blocker — but cheapest to do now, while the copies are still identical.

5. Smaller notes

  • Weak test: successful device authorize transitions to Ready state asserts Ready || Complete and only checks userCode inside an if, so it can go green without ever seeing Ready. It copies the existing sign-in test, but the new copy could be deterministic.
  • Naming leak: the create screen and the podcast modal both consume TvSignInUiState. With three consumers now, TvDeviceAuthUiState (next to deviceAuthFlow) would read better.
  • Code expiry unhandled: deviceAuthFlow ignores expiresIn and polls forever. This matters more on the create screen than on sign-in — signing up involves email entry and possibly verification, so hitting the 1800s expiry is plausible, and the user then gets a bare "Something went wrong" + Retry with no hint that the code aged out. Pre-existing, newly relevant.
  • A11y: TvSignInQrContent's QrCode passes contentDescription = null. The old create screen described its QR. The step text carries the URL so TalkBack users aren't stranded, but a description would be better. Pre-existing in the base branch.
  • Test nit: TvCreateAccountViewModelTest:24 stubs the network failure with the message "qr disabled", which describes something else.
  • Missing coverage: nothing asserts retry() re-requests a device code after an error in the create VM (the sign-in VM has that test), and nothing covers the screen's navigate-on-Complete.

Verification caveat: Gradle is not runnable in this environment (the command needed approval), so I did not run spotlessCheck, the unit tests, or lint. Everything above is from reading the code. The lint concern in #1 in particular needs a real run to confirm or dismiss.
• branch feat/tv-create-account-device-auth

Comment thread modules/services/localization/src/main/res/values/strings.xml
@sztomek sztomek changed the title [TV] Create account via device auth with auto-advance to syncing [TV] Actively poll API in create account flow Aug 26, 2026
@sztomek sztomek added this to the 8.20 milestone Aug 26, 2026
@sztomek
sztomek marked this pull request as ready for review August 26, 2026 12:18
@sztomek
sztomek requested a review from a team as a code owner August 26, 2026 12:18
@sztomek
sztomek requested review from geekygecko and removed request for a team August 26, 2026 12:18
@claude

claude Bot commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @sztomek's task in 7m 53s —— View job


Review: TV create account via device auth

  • Read the PR diff against feat/tv-onboarding-analytics
  • Check whether the earlier review findings were addressed
  • Review device-auth plumbing (SyncManager / SyncManagerImpl)
  • Review the TV create-account screen + ViewModel
  • Review tests & string removals
  • Attempt to run tests / lint — blocked, see caveat
  • Post review

Re-reviewed on ready for review, against origin/feat/tv-onboarding-analytics. The change is well-shaped and I found no correctness bug in the pairing flow: reusing deviceAuthFlow + TvSignInQrContent is the right call, rememberUpdatedState around the completion callback is handled correctly, LaunchedEffect(uiState) can't double-fire because the key stays Complete, and navigateClearingBackStack(SYNCING) matches the sign-in route so a user can't back into a spent pairing screen. Removed strings have no surviving references anywhere in .kt/.xml/.java.

First, a correction. In my earlier pass I claimed dropping the two strings from the default locale could newly fail ExtraTranslation and asked you to run lint. That is almost certainly wrong — modules/services/localization/build.gradle.kts:11-16 points the module at lint-no-op.xml (<issue id="all" severity="ignore"/>) specifically so checkDependencies from app doesn't reach it, and app/lint-baseline.xml has zero entries from that module while carrying plenty from other dependency modules. No action needed. Details and the retraction are on the line itself.

New this round

# Where Issue
1 SyncManagerImpl.kt:249 user_account_created lands with flow = unknown, breaking the funnel join with create_account_shown(flow = initial_onboarding)
2 SyncManagerImplTest.kt:97 Type-only event assertion hides the payload — and hid #1

The one I'd actually act on is #1. Flipping isNewAccount to true routes into the UserAccountCreatedEvent branch of trackSignIn, which hardcodes flow = OnboardingFlowType.Unknown (SyncManagerImpl.kt:587). So the shipped TV funnel is create_account_shown(flow = initial_onboarding) → user_account_created(flow = unknown). flow = Unknown is pre-existing and platform-wide (trackRegister at line 633 does the same for mobile password signup), so this isn't a regression you introduced — but the commit immediately below this one in the stack (60c791a) exists precisely to get flow right for TV onboarding events, and the PR body presents this as the third of the three events from #5773, so the mismatch is worth either fixing or calling out explicitly.

Two smaller new observations, neither blocking:

  • source_in_code on the follow-modal path. deviceAuthFlow hardcodes SignInSource.UserInitiated.Onboarding, so TvPodcastDetailsViewModel now emits user_account_created(source = qr_code, source_in_code = onboarding, flow = unknown) from a podcast-details surface. The source_in_code value is pre-existing in the base branch; it just never attached to a creation event before.
  • create_account_shown from Home. TvOnboardingNavHost.kt:83 reaches CREATE_ACCOUNT from TvScaffold — a signed-out user already past onboarding — but trackShown() hardcodes OnboardingFlowType.InitialOnboarding. Base-branch behaviour, but this PR is the one wiring up the screen it describes.

Still open from the earlier pass

Unchanged in 2d7084b, and all still fine to defer:

  • Attribution inflation in the follow modal (comment context, original on TvPodcastDetailsViewModel.kt:103) — the modal's own copy is "Create your account or log in" and it's shown to any signed-out user following a podcast, so existing users logging in there now count as creations. At minimum please record the tradeoff in a code comment rather than only in the PR body.
  • isNewAccount semantics (SyncManager.kt:71) — it lands in AuthResultModel.isNewAccount, which is response.isNew everywhere else and drives paywall routing in OnboardingFlowRoutes.kt. No consumer is affected today (both TV call sites discard the result); a KDoc line and/or dropping the = false default would stop a future reader trusting it as server truth.
  • ~150 duplicated lines (TvCreateAccountScreen.kt) — TvCreateAccountError is still byte-identical to TvSignInError; TvCreateAccountLoading/TvCreateAccountContent/createAccountSteps each differ from their sign-in twin by one string; the VM's polling block is line-for-line. Cheapest to collapse into a parameterised TvDeviceAuthScreen now, while the copies are still identical. The three new previews doubled the duplicated preview count too.
  • Weak test (TvCreateAccountViewModelTest.kt:54) — assertTrue(state is Ready || state is Complete) with the userCode check inside an if. MainCoroutineRule uses UnconfinedTestDispatcher, so in practice the first item is Ready and the test does assert something — but nothing in the test enforces that, and the while (state !is Complete) loops below report regressions as a 3s Turbine timeout rather than an assertion diff.
  • expiresIn unhandled — deviceAuthFlow polls forever. More relevant on the create screen than on sign-in, since signup involves email entry and possibly verification, and the user gets a bare "Something went wrong" with no hint the code aged out. Pre-existing.
  • Naming — TvSignInUiState now has three consumers; TvDeviceAuthUiState next to deviceAuthFlow would read better. Also createAccountSteps reuses tv_create_account_modal_step_create on a full screen, so the _modal_ in the resource name is now a lie.
  • Nits — TvCreateAccountViewModelTest:24 stubs the failure as "qr disabled", which describes something else; TvSignInQrContent's QrCode passes contentDescription = null (pre-existing; the step text carries the URL, so TalkBack users aren't stranded).
  • Coverage gaps — nothing asserts retry() re-requests a device code in the create VM (the sign-in VM has that test), and nothing covers navigate-on-Complete.

One thing worth noting since it isn't obvious: .buildkite/commands/lint.sh runs lint on app, automotive and wear only — the tv module is never linted in CI, and tv/build.gradle.kts:37-39 sets checkDependencies = false. So the new screen won't be seen by any lint run. Out of scope here, but relevant to how much confidence CI green gives you on TV code.

Verification caveat

Gradle is not runnable in this environment — ./gradlew :tv:testDebugUnitTest and :app:lintRelease both required approval that isn't available here. So I did not run the unit tests, spotlessCheck, or lint; everything above is from reading the code and the build configuration. The lint retraction in particular is a config-and-baseline argument, not an observed lint run.
• branch feat/tv-create-account-device-auth

Comment thread modules/services/localization/src/main/res/values/strings.xml
@sztomek
sztomek force-pushed the feat/tv-onboarding-analytics branch from 9c743d2 to 1fb257a Compare August 27, 2026 08:07
Base automatically changed from feat/tv-onboarding-analytics to main August 27, 2026 08:32
@sztomek
sztomek force-pushed the feat/tv-create-account-device-auth branch from 2d7084b to 578a1ab Compare August 27, 2026 09:11
@claude

claude Bot commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

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


Review: TV create account via device auth (incremental)

Reviewed only 578a1ab Address PR comments — the delta since my last pass (the three commits below it are the rebased versions of what I already reviewed). Nothing new is blocking, and the changes close most of what was open: TvPodcastDetailsViewModel.kt:103 now passes isNewAccount = false, so user_account_created fires only from the create screen and the mixed create/login follow modal no longer inflates it; the = false defaults are gone from both SyncManager.loginWithDeviceAuth and deviceAuthFlow with a KDoc stating it is a caller assertion, and all three call sites plus the mocks are updated; the SyncManagerImplTest assertions now pin the full event payloads in both directions and assert the sink drains; and TvCreateAccountViewModelTest collects until Complete and asserts Ready.userCode unconditionally, so it can no longer pass without observing Ready.

One consequence worth noting rather than fixing: flow = OnboardingFlowType.Unknown on the created event is now explicitly asserted in SyncManagerImplTest.kt:99, which pins the shipped create_account_shown(initial_onboarding) → user_account_created(unknown) mismatch as intended behaviour. That's platform-wide and pre-existing (trackRegister does the same), so fine to leave — just be aware the funnel can't be joined on flow.

Non-blocking (still open, unchanged)

  • tv/src/main/java/au/com/shiftyjelly/pocketcasts/onboarding/createaccount/TvCreateAccountScreen.kt — TvCreateAccountError is still byte-identical to TvSignInError, and TvCreateAccountLoading/TvCreateAccountContent/createAccountSteps/the VM's polling block still differ from their sign-in twins by one string each; collapsing into a parameterised TvDeviceAuthScreen is cheapest while the copies are identical (thread). Fix this →
  • tv/src/main/java/au/com/shiftyjelly/pocketcasts/onboarding/signin/TvDeviceAuth.kt — expiresIn is still ignored, so polling runs forever and an expired code surfaces as a bare "Something went wrong" + Retry. Pre-existing, more reachable now that the create screen (email entry, possibly verification) uses the same flow.

Nits

  • TvSignInUiState now has three consumers; TvDeviceAuthUiState, next to deviceAuthFlow, would read better.
  • SyncManagerImplTest.kt:104 hardcodes redirectPath = "none" where SyncManagerImpl.NO_REDIRECT_PATH exists.

I did not run Gradle — ./gradlew :tv:testDebugUnitTest and spotlessCheck aren't runnable in this environment, so this is from reading the code.
• branch feat/tv-create-account-device-auth

@sztomek
sztomek merged commit 1094606 into main Aug 27, 2026
20 checks passed
@sztomek
sztomek deleted the feat/tv-create-account-device-auth branch August 27, 2026 10:39
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