[TV] Return to the welcome screen after signing out - #5744
Conversation
|
Claude finished @sztomek's task in 4m 3s —— View job Code review — PR #5744
VerdictThe change is small, well-motivated and the description's factual claims check out. I verified the three load-bearing ones:
Also correct: No security or performance concerns — no new I/O, no new persistence, one extra lambda parameter. Findings1. Medium — a failed or partial sign-out now strands the user in an empty Home rather than self-healing to Welcome. (inline) The account removal and the data wipe can fail independently. Under the old gate that combination self-corrected (the wiped onboarding flag forced Welcome). Under the new gate it resolves to 2. Low/Medium — navigating to Welcome immediately makes the sign-in-races-the-wipe window easy to reach. (inline)
3. Nit — duplicated clear-the-back-stack navigation, plus a state-restoration caveat. (inline)
4. Test coverage. The two rewritten Not verifiedI could not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| syncManager: SyncManager, | ||
| ) : ViewModel() { | ||
| val startDestination: String = if (settings.hasCompletedOnboarding()) { | ||
| val startDestination: String = if (syncManager.isLoggedIn()) { |
There was a problem hiding this comment.
A failed/partial sign-out now strands the user in an empty Home instead of self-healing to Welcome.
The account removal is asynchronous and can fail independently of the data wipe:
TvSignOutManager.signOutAndWipeData()callsUserManager.signOutAndClearData(), which launchessyncManager.signOut { … }onapplicationScope(UserManager.kt:167).SyncManagerImpl.signOut()runssyncServiceManager.signOut()→action()(network + analytics + experiment refresh) → only thensyncAccountManager.signOut()(SyncManagerImpl.kt:191-196). If anything before that line throws, the account is never removed andsyncManager.isLoggedIn()staystrue.- Meanwhile
TvSignOutManagergives up afterSIGN_OUT_TIMEOUT(10s) and unconditionally proceeds to delete downloads,settings.clearUserPreferences()andtvPreferences.clearAll().
Under the old gate this was self-correcting: clearUserPreferences() does not preserve DONE_INITIAL_ONBOARDING_KEY (SettingsImpl.kt:592-613), so the next launch showed Welcome regardless of whether the token removal succeeded. With the new gate, "logged in + wiped database" resolves to HOME, which is exactly Apple TV's .dataLossResync state the description declares out of scope — on tvOS the keychain delete is synchronous, so it can't half-happen there; here it can.
PROCESSED_SIGNOUT_KEY is written synchronously (UserManager.kt:193), reset to false on every login (SyncManagerImpl.kt:241, :729), defaults to true, and is preserved across clearUserPreferences() — so it's a cheap durable guard:
| val startDestination: String = if (syncManager.isLoggedIn()) { | |
| val startDestination: String = if (syncManager.isLoggedIn() && !settings.getFullySignedOut()) { |
(requires keeping the Settings injection). Alternatively, leave the gate as-is and accept the trade-off — but it'd be worth a LogBuffer entry so the state is diagnosable.
| onLogOut = { | ||
| isProfileModalVisible = false | ||
| viewModel.signOut() | ||
| onSignedOut() |
There was a problem hiding this comment.
viewModel.signOut() is fire-and-forget (signOutManager.signOutAndWipeData() just launches on @ApplicationScope), and onSignedOut() fires synchronously right after. Two consequences worth thinking about:
-
Navigation happens before the auth state actually flips.
syncManager.isLoggedIn()/isLoggedInObservableonly becomefalseat the end ofSyncManagerImpl.signOut()(SyncManagerImpl.kt:195). Harmless today becauseTvWelcomeScreendoesn't read login state, but any future screen onLANDINGthat does will see a staleSignedIn. -
Sign-in can now race the tail of the wipe. The wipe keeps running for up to ~10s+ after this returns and ends with
settings.clearUserPreferences()+tvPreferences.clearAll(). Welcome auto-focuses Sign In, so the user is one click from starting a fresh device-auth flow while the old wipe is still in flight — if the new login lands first, those tail steps clear the new session's preferences. The hazard pre-dates this PR (the profile modal already offered Log In after logout), but dropping the user straight onto the Welcome CTA makes it much easier to hit.
If you want to close it, having TvSignOutManager expose the wipe Job/a StateFlow<Boolean> and navigating on completion (with the existing spinner/blocking UI) would be the tighter version.
| onSignedOut = { | ||
| navController.navigate(TvOnboardingRoutes.LANDING) { | ||
| popUpTo(navController.graph.id) { inclusive = true } | ||
| } | ||
| }, |
There was a problem hiding this comment.
The pop-whole-graph pattern is correct here (the start destination can be HOME, so popUpTo(LANDING) wouldn't work) and matches the SIGN_IN handler at line 58-60. Since it now appears twice, a small local helper would keep the two in sync:
val navigateClearingBackStack: (String) -> Unit = { route ->
navController.navigate(route) {
popUpTo(navController.graph.id) { inclusive = true }
}
}Nit only — no behavioural change requested.
One thing to be aware of for testing step 4: rememberNavController() saves the back stack, so a signed-out "browse without account" session that is restored after process death (rather than a genuine cold launch) will come back on HOME, not Welcome. Probably the desired state-restoration behaviour, just not quite the "every signed-out launch shows Welcome" invariant the description states.
sztomek
changed the title
|
Claude finished @sztomek's task in 4m 0s —— View job Code review — PR #5744
VerdictThe two fixes from the last round landed cleanly and I re-verified both. One new finding, though — it's the flip side of removing What I confirmed:
Findings1. Medium — interrupting the first sync now permanently skips it. (inline) The old flag was set after So: sign in → Nothing heals it. Under the old gate this same interruption was also wrong (Welcome shown to a logged-in user) but it self-healed. The new gate converts a recoverable state into a stuck one. Either resolve to 2. Low/Medium — sign-in still races the tail of the wipe (carried over, not addressed).
The gate itself is safe from this — Related and unchanged: 3. Nit — the RxJava commit isn't in the description. 4. Nit — dead stub in the new test. (inline) 5. Test coverage. The three Not verifiedI couldn't run |
| settings: Settings, | ||
| ) : ViewModel() { | ||
| val startDestination: String = if (settings.hasCompletedOnboarding()) { | ||
| val startDestination: String = if (syncManager.isLoggedIn() && !settings.getFullySignedOut()) { |
There was a problem hiding this comment.
Interrupting the first sync now permanently skips it. This is the one case where dropping completeOnboarding() loses information the login state can't reconstruct.
The old flag was set after onSyncComplete, so it encoded "signed in and initial sync finished". isLoggedIn() && !getFullySignedOut() only encodes the first half — the account is added at SyncManagerImpl.kt:233-241 (loginWithDeviceAuth), i.e. before the user ever reaches SYNCING.
So:
- Sign in → account added,
setFullySignedOut(false)written → navigate toSYNCING. - User presses Back (which exits, since
onSignInCompletepopped the whole graph) or the process is killed mid-sync.TvSyncingViewModel.startSync()runs inviewModelScope(TvSyncingViewModel.kt:87), so the refresh is cancelled. - Cold launch → gate resolves to
HOME.SYNCINGis only reachable fromonSignInComplete, so the sync never runs again.
And nothing else heals it: :tv never calls RefreshPodcastsTask.scheduleOrCancel (only app, wear and AdvancedSettingsViewModel do), and TvHomeViewModel only reads local/discover data. refreshPodcastsAfterSignIn() has already called clearLastRefreshTime() + markAllPodcastsUnsynced() (PodcastManagerImpl.kt:218-222), so the DB is left mid-migration with no scheduled worker to finish it. The user sits on an empty Home, signed in, with no way back to SYNCING short of logging out and in again.
Under the old gate this interruption was also wrong (Welcome shown to a logged-in user) but it self-healed — signing in again re-ran the sync. The new gate turns a recoverable state into a stuck one.
Two ways out, either is fine:
- Route to
SYNCINGrather thanHOMEwhen logged in but the initial sync hasn't been recorded — keeps the parity goal (signed-out ⇒ Welcome) while preserving the resume behaviour. - Or trigger
RefreshPodcastsTask.scheduleOrCancel/ a one-shot refresh whenHOMEis the start destination, which also fixes the broader "TV never refreshes in the background" gap.
| assertEquals(TvOnboardingRoutes.LANDING, viewModel.startDestination) | ||
| fun `start destination is landing when signed out`() { | ||
| whenever(syncManager.isLoggedIn()).thenReturn(false) | ||
| whenever(settings.getFullySignedOut()).thenReturn(true) |
There was a problem hiding this comment.
Nit: this stub is dead. && short-circuits on isLoggedIn() == false, so getFullySignedOut() is never called here — the test passes with or without the line, and would also pass with .thenReturn(false). Harmless today (no MockitoJUnitRunner/strict-stubs rule in this class, so no UnnecessaryStubbingException), but it reads as though the assertion depends on both values when it only depends on one. Dropping it makes the signed-out case unambiguous.
|
Version |
2 participants
Description
Brings the Android TV logout/launch flow to parity with the Apple TV app.
Problem: After signing out on Android TV, the next cold launch dropped the user on the Welcome/landing screen inconsistently. The launch destination was gated on
Settings.hasCompletedOnboarding()(DONE_INITIAL_ONBOARDING_KEY), which sign-out wipes viaclearUserPreferences(). So "browse without account" persisted across launches (skipped Welcome forever) while a sign-out did not — and logging out in-session silently left you on the Home scaffold rather than returning to Welcome.Apple TV behavior (parity target):
AppCoordinatorgates the launch screen purely onisLoggedIn(keychain). "Browse without account" is ephemeral in-memory state that never persists, andlogout()wipes data then immediately returns to the Welcome screen. Every signed-out cold launch shows Welcome.This change:
TvOnboardingViewModel.startDestinationnow gates onSyncManager.isLoggedIn()instead ofSettings.hasCompletedOnboarding(). The now-vestigialcompleteOnboarding()method and its twoTvOnboardingNavHostcall-sites are removed (nothing else in:tvreads the onboarding flag). Browse-without-account is now per-session, matching tvOS.TvScaffoldgains anonSignedOutcallback; the profile modal's Log Out now callssignOut()and then navigates back to the Welcome (LANDING) screen, clearing the Home back-stack — mirroring Apple TV'slogout() { … state = .welcome }.The async data wipe continues to run in
@ApplicationScope, unaffected by the navigation/ViewModel teardown.Out of scope: Apple TV's
.dataLossResyncstate (fresh DB but keychain still logged in → forced re-sync spinner) has no Android TV equivalent and is not added here.Testing Instructions
Screenshots or Screencast
Screen_recording_20260814_162130.mp4
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xml