Skip to content

[TV] Show an alert when the server signs the user out - #5783

Merged
sztomek merged 5 commits into
mainfrom
feat/tv-signed-out-alert
Aug 27, 2026
Merged

sztomek merged 5 commits into
mainfrom
feat/tv-signed-out-alert

Conversation

@sztomek

@sztomek sztomek commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

Description

Adds a server-forced sign-out alert to the Android TV app, matching Apple TV. When a user is signed out by the server (token revoked / refresh failed / account removed) rather than by their own action, iOS shows a full-screen alert instead of silently dropping them; Android TV previously surfaced nothing (only a system notification, which is invisible on a TV). This closes functional gap #5 from the Android-TV-vs-iOS-tvOS onboarding parity audit and lands the deferred signed_out_alert_shown analytics event (one of the three events split out of the onboarding-analytics PR #5773).

How it works

  • Signal at the source (shared): UserManager gains onServerSignOut: SharedFlow<Unit>, emitted from signOut() only when wasInitiatedByUser == false (the Android equivalent of iOS's user_initiated). Transient (replay = 0) = in-session only, matching iOS.
  • TV observes it: TvOnboardingNavHost collects the flow and navigates to a new SIGNED_OUT route from any screen.
  • Alert screen: TvSignedOutScreen (Compose + tv-material3, styled like TvWelcomeScreen) shows "You've been logged out" + message + a Log in button; signed_out_alert_shown fires on appear. The button fully wipes local data (via TvSignOutManager) and returns to Welcome — matching iOS's logout().
  • No double-count: TokenErrorNotification (which already fires SignedOutAlertShownEvent) is suppressed on TV, so the in-app alert is the single event source.

A user-initiated Log Out (Profile) is unaffected — it passes wasInitiatedByUser = true, so no alert and no emit.

Known limitation (conscious decision)

Because the TV token-error notification is suppressed, a forced sign-out that surfaces only via the OkHttp interceptor's swallowed RefreshTokenExpiredException (rather than RefreshPodcastsThread or the account-removed listener) won't show the alert until the next podcast refresh runs (refreshOnLaunch + periodic). The primary forced-sign-out paths are covered; this is a delay, not a permanent gap.

Fixes POC-867 https://linear.app/a8c/issue/POC-867/server-forced-signout-alerts

Testing Instructions

  1. Sign in on Android TV.
  2. Trigger a server-forced sign-out (revoke the session server-side / let the refresh token expire so the next podcast refresh fails).
  3. The "You've been logged out" alert appears over whatever screen you were on.
  4. Press Log in → local data is wiped and you land on the Welcome screen.
  5. Confirm a normal Profile → Log Out does not show the alert (it goes straight to Welcome as before).

Screenshots or Screencast

Screenshot_20260826_125009

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. (signed_out_alert_shown already exists 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 4m 26s —— View job


Review: [TV] Show an alert when the server signs the user out

  • Gather context (read diff + surrounding code)
  • Review correctness / potential bugs
  • Review code quality, tests, security & performance
  • Post review feedback (6 inline comments)

Nice, well-scoped change — the signal-at-the-source approach is the right shape, the TV suppression is correctly scoped (Util.isTv reads pocketcasts_tv manifest metadata, so automotive/wear are untouched), UserManager is @Singleton so the SharedFlow instance really is shared, and TvSignedOutScreen matches TvWelcomeScreen's styling and focus pattern closely. Tests cover both ViewModels and all three signOut branches.

Findings below, roughly by importance.


1. Background forced sign-outs lose both the alert and the event

MutableSharedFlow(replay = 0, extraBufferCapacity = 1) drops emissions when there are no subscribers — extraBufferCapacity only buffers for slow subscribers, it does not hold a value for a future one. The only TV subscriber is the LaunchedEffect in TvOnboardingNavHost, alive only while composed.

The dominant TV path is RefreshPodcastsThread, which also runs from the periodic background worker while the app isn't running. In that case the emit is dropped, TokenErrorNotification is now suppressed on TV so no signed_out_alert_shown fires either, and on next launch startDestination resolves to LANDING (because getFullySignedOut() is now true) — the user lands on Welcome with no explanation.

The PR frames this as "a delay, not a permanent gap," but for the background-refresh case it is permanent: nothing replays the signal on next launch. Since the parity metric here is the analytics event, this is worth closing — e.g. persist a pendingSignedOutAlert flag in TvPreferences and route to SIGNED_OUT at startup when set. Details →

2. Repeat emissions re-fire signed_out_alert_shown

navigateClearingBackStack pops the graph inclusively then navigates, so a second emission while already on SIGNED_OUT builds a new NavBackStackEntry. CallOnce is rememberSaveable-backed and scoped to that entry, so it re-fires — the exact double-count the TokenErrorNotification suppression exists to prevent (note that notification has an explicit 2s debounce for this reason). Two signOut(wasInitiatedByUser = false) call sites in RefreshPodcastsThread plus refreshOnLaunch + periodic refresh make this realistic. Suggested guard →

3. The Log in button emits a contradictory second UserSignedOutEvent

signOutAndWipeData() → signOutAndClearData(wasInitiatedByUser = true) → signOut(wasInitiatedByUser = true), which passes the guard even though fullySignedOut is already true, and tracks UserSignedOutEvent(userInitiated = true). So a single server-forced sign-out produces two events — userInitiated = false then userInitiated = true — skewing the very split this feature is meant to measure. Details →

4. Emit ordering vs. the in-flight sign-out job

tryEmit fires before settings.setFullySignedOut(true) and before the async signOut job is launched. The alert can be on screen with its button pressable while the original job is still running, so signOutAndWipeData() can race it. Moving the emit to just before return signOutJob makes this deterministic.

5. Smaller items

  • rememberUpdatedState(onLogIn) in TvSignedOutScreen.kt:46 is a no-op — the lambda isn't captured by a long-lived effect. Suggestion →
  • "Tap the button below" on a TV — no touch input. Worth fixing before GlotPress picks it up. Suggestion →
  • UserManagerImplTest builds StandardTestDispatcher() with its own scheduler, so the applicationScope.launch inside signOut() never runs. Harmless for these assertions, a trap for the next person. Suggestion →
  • No BackHandler on SIGNED_OUT. Since the back stack is cleared, pressing Back exits the app outright — reasonable for a modal alert, but worth a conscious decision (Apple TV alerts are typically non-dismissible, so this may be intended).
  • TvSignedOutViewModel.logOut() has no re-entrancy guard; a double-press launches two concurrent wipes.

6. PR description nit

The description lists "the account-removed listener" among the covered paths, but beginMonitoringAccountManager is only called from PocketCastsApplication, AutomotiveApplication and PocketCastsWearApplication — not TvApplication. On TV the only live emit source is RefreshPodcastsThread. Doesn't change the code, but it narrows the coverage claim and reinforces finding #1.


Security / performance: nothing concerning. No new network or storage surface; the flow carries Unit only; Util.isTv does a PackageManager metadata lookup per show() call (uncached, unlike getAppPlatform) but the call rate is negligible.

Not run: I did not execute ./gradlew spotlessCheck or the unit tests in this environment — CI covers both.
· branch feat/tv-signed-out-alert

Comment thread modules/services/localization/src/main/res/values/strings.xml Outdated
@sztomek sztomek added [Type] Feature Adding a new feature. [Area] Login User login [Area] TV labels 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 10:53
@sztomek
sztomek requested a review from a team as a code owner August 26, 2026 10:53
@sztomek
sztomek requested review from geekygecko and removed request for a team August 26, 2026 10:53
@claude

claude Bot commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

Review: [TV] Show an alert when the server signs the user out

Working…

  • Read the diff and surrounding code
  • Verify correctness concerns
  • Review quality, tests, security, performance
  • Post review

View job run · branch feat/tv-signed-out-alert

@claude

claude Bot commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

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


Review: [TV] Show an alert when the server signs the user out

  • Read the diff and surrounding code
  • Verify correctness concerns
  • Review quality, tests, security, performance
  • Post review

Note: this synchronize event didn't change the diff — origin/main...HEAD is byte-identical to what I reviewed 15 minutes ago (tip still 99b722b, same 4 commits, same 11 files / +294/-1). I re-read the code and surrounding call sites rather than trusting the earlier pass, and re-verified each finding against the source. All of them still hold, so I'm not re-posting duplicate inline comments — the existing threads are the place to reply.


Re-verified findings

1. Background forced sign-outs lose both the alert and the event — MutableSharedFlow(extraBufferCapacity = 1) with default replay = 0 drops emissions when there are no subscribers; extraBufferCapacity only buffers for slow subscribers. The only TV subscriber is TvOnboardingNavHost.kt:43's LaunchedEffect. Confirmed the emit sites: RefreshPodcastsThread.kt:155 and :272, plus UserManager.kt:109 — and I confirmed beginMonitoringAccountManager is called only from PocketCastsApplication, AutomotiveApplication and PocketCastsWearApplication, never TvApplication, so on TV RefreshPodcastsThread is the only live emit source. When it fires from the periodic worker with the app not composed: emit dropped → TokenErrorNotification suppressed → no signed_out_alert_shown → next launch resolves to LANDING with no explanation. For that path it's a permanent gap, not a delay. Thread → · Fix this →

2. Repeat emissions re-fire signed_out_alert_shown — confirmed navigateClearingBackStack (TvOnboardingNavHost.kt:33-37) does popUpTo(graph.id) { inclusive = true } then navigate, and CallOnce (ComposeUtils.kt:16-22) is rememberSaveable-backed, so a second emission while already on SIGNED_OUT builds a fresh NavBackStackEntry and re-fires the event — the exact double-count the notification suppression exists to prevent (that notification has an explicit 2s debounce for the same reason). Two emit sites in RefreshPodcastsThread plus refreshOnLaunch + periodic refresh make this reachable. Thread with suggestion →

3. The Log in button emits a contradictory second UserSignedOutEvent — confirmed in TvSignOutManager.kt:46: wasInitiatedByUser = true. Since UserManager.kt:174's guard is wasInitiatedByUser || !settings.getFullySignedOut(), the wipe re-enters signOut even though fullySignedOut is already true, tracking UserSignedOutEvent(userInitiated = true). One server-forced sign-out → two events, false then true, skewing the very split this feature measures. Thread →

4. Emit ordering vs. the in-flight sign-out job — tryEmit at UserManager.kt:176 runs before settings.setFullySignedOut(true) (line 205) and before applicationScope.launch. The alert can be on screen with a pressable button while the original job is still running, so signOutAndWipeData() races it (and TvSignOutManager only joins the job it started). Moving the emit to just before return signOutJob makes it deterministic.

5. Smaller items (all still present)

  • rememberUpdatedState(onLogIn) at TvSignedOutScreen.kt:46 is a no-op — no long-lived effect captures it. Thread →
  • "Tap the button below" in strings.xml:277 — no touch input on TV. Worth fixing before GlotPress picks it up. Thread →
  • UserManagerImplTest.kt:67 passes a bare StandardTestDispatcher() with its own scheduler, so the applicationScope.launch in signOut() never dispatches. Harmless for these three assertions, a trap for the next person. Thread →
  • No BackHandler on SIGNED_OUT; with the back stack cleared, Back exits the app. Reasonable for a modal alert — just flagging it as a conscious call.
  • TvSignedOutViewModel.logOut() has no re-entrancy guard; a fast double-press launches two concurrent wipes.

6. PR description — the description lists "the account-removed listener" as a covered path, but as noted in #1 that listener is never started on TV. Narrows the coverage claim.


What's good: signal-at-the-source is the right shape; Util.isTv(context) reads pocketcasts_tv manifest metadata so the suppression is correctly scoped and leaves automotive/wear alone; UserManager is @Singleton so the SharedFlow really is shared; TvSignedOutScreen mirrors TvWelcomeScreen's styling and focus pattern; TvSignOutManager's runWipeStep error handling is solid; tests cover both ViewModels and all three signOut guard branches.

Security / performance: nothing concerning. No new network or storage surface, the flow carries Unit only, and Util.isTv does an uncached PackageManager metadata lookup per show() call at a negligible rate.

Not run: I did not execute ./gradlew spotlessCheck or the unit tests in this environment — CI covers both.
· branch feat/tv-signed-out-alert

@geekygecko geekygecko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might work well with the changes to the account authentication PR. #5775

@claude

claude Bot commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

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


Review: [TV] Show an alert when the server signs the user out

Reviewed only c1994a0 Address PR comments against my last pass at 99b722b. That commit closes findings 2, 3, 4 and all three of the smaller items — the route guard in TvOnboardingNavHost prevents the duplicate signed_out_alert_shown, the emit now sits after setFullySignedOut(true), rememberUpdatedState is gone, the string says "Select", and StandardTestDispatcher(testScheduler) is wired to runTest's scheduler. One finding from the last pass is still open, and the fix for the duplicate UserSignedOutEvent works through a side effect worth making explicit.

Blocking

  • modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/user/UserManager.kt:100 — the MutableSharedFlow still has replay = 0, so a forced sign-out with no active subscriber loses both the alert and the signed_out_alert_shown event permanently; thread still open at #discussion_r3861950544. Correcting my earlier framing of the mechanism: TV never calls RefreshPodcastsTask.scheduleOrCancel, so there is no periodic worker — the reachable window is refreshOnLaunch() → podcastManager.refreshPodcasts("tv launch") → RefreshPodcastsTask.runNow, which runs in applicationScope in-process. If the user leaves the app while that refresh is in flight, TvOnboardingNavHost is decomposed, the emit is dropped, TokenErrorNotification is suppressed on TV, and the next launch resolves to LANDING with no explanation. Narrower than I first said, still a permanent loss rather than a delay. Fix this →

Non-blocking

  • tv/src/main/java/au/com/shiftyjelly/pocketcasts/onboarding/signedout/TvSignedOutViewModel.kt:21 — wasInitiatedByUser = false suppresses the duplicate event by making UserManagerImpl.signOut a complete no-op (getFullySignedOut() is already true), which also drops the returned job so TvSignOutManager's join()/timeout no longer guards the local wipe against the still-in-flight forced-sign-out job; a wipeLocalDataOnly() variant or a comment at the call site would make the intent explicit. #discussion_r3870200830
  • tv/src/main/java/au/com/shiftyjelly/pocketcasts/onboarding/signedout/TvSignedOutViewModel.kt:20 — still no re-entrancy guard on logOut(); a fast double-press launches two concurrent wipes. Previously raised, unchanged.
    · branch feat/tv-signed-out-alert

@sztomek
sztomek merged commit 1012ada into main Aug 27, 2026
21 checks passed
@sztomek
sztomek deleted the feat/tv-signed-out-alert branch August 27, 2026 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Area] Login User login [Area] TV [Type] Feature Adding a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants