[TV] Add email/password sign-in option - #5782
Conversation
Generated by 🚫 Danger |
|
Claude finished @sztomek's task in 9m 29s —— View job Code review:
|
| State | Rendered result |
|---|---|
| Focus on the unselected tab | Selected tab's label is textPrimary 0xFFFBFBFC on a backgroundActive 0xFFFBFBFC pill → invisible label |
| Focus on the unselected tab | focusedContentColor == inactiveContentColor == contentColor and the pill stays pinned to selectedIndex → no focus indicator at all |
| Focus outside the row (typing in the form) | Track backgroundBase 0xFF292B2E vs inactiveColor backgroundBase 0xFF292B2E → selected pill invisible |
The second row is the serious one: the interaction this PR requires (move focus to Email, press OK) gives the user no feedback that focus has arrived. The emulator screenshots in the description wouldn't reveal any of these, since they capture the focused-and-selected state only.
2. Soft keyboard is shown but never hidden — TvEmailSignInForm.kt:163-183
keyboardController?.show() on focus gain has no matching hide() on focus loss, and none on onDone. On a leanback IME the keyboard will sit over the Log in button, the inline validation errors, and the server error message — i.e. over precisely the feedback this PR adds. The description lists the keyboard behaviour as on-device follow-up; I'd treat the missing hide() as a defect to fix now rather than something to verify.
Same comment covers onPreviewKeyEvent returning true unconditionally instead of propagating moveFocus's result, which swallows D-pad presses the focus system declined.
Should fix
3. Email isn't trimmed — TvSignInViewModel.kt:55-57. The phone does email.trim(); the anchored EMAIL_REGEX.matches() rejects a trailing space. TV keyboards produce trailing spaces routinely, and a trailing space is invisible from the sofa — the user sees "Please enter a valid email" on a field that looks fine.
4. Credentials and isSubmitting are never cleared on success — TvSignInViewModel.kt:77-91. TvEmailSignInState is a data class, so the generated toString() renders the plaintext password — one future Timber.d("$state") or failing assertEquals and it's in a log. Also: submitEmailSignIn relies on selectMode(Email) having cancelled pollingJob rather than cancelling it itself, leaving two live paths to Complete.
5. Returning to the QR tab mints a new device code — TvSignInViewModel.kt:41-53. A user who already entered the displayed code on their phone, then toggled to Email and back, gets their code invalidated. Leaving the poll running across mode switches avoids this entirely.
6. LoadingView fills all remaining height — TvSignInScreen.kt:219-222. LoadingView applies fillMaxSize() internally and the wrapper Box only sets a minimum height, so the QR loading state stretches and the logo/title/tabs visibly jump when Ready arrives.
7. Retry button lost its auto-focus — TvSignInScreen.kt:226-248. main's TvSignInError focused Retry via a FocusRequester; the refactor dropped it and gave initial focus to the tab row instead. Also worth confirming on-device that Down from TabRow traverses into the button at all, given TabRow's own focus group.
8. Form stays editable during submit; no offline handling — TvEmailSignInForm.kt:110-125. The phone gates fields on enableSubmissionFields and pre-checks Network.isConnected to show log_in_no_network. Neither is carried over.
Nits
- Fourth copy of credential validation —
TvSignInViewModel.kt:105-112. The hand-rolled regex is stricter thanPatterns.EMAIL_ADDRESS, so a handful of addresses that work on the phone are rejected on TV. If the regex exists to dodgeandroid.util.Patternsin JVM unit tests (I assume it does), please say so in a comment so nobody "fixes" it back. - State modelling — the screen collects three interdependent flows (
uiState,mode,emailState) and the email path writesCompleteinto what is otherwise the QR device-auth state machine. AGENTS.md asks for a single immutable state; oneTvSignInUiStatecarryingmodewould make "polling is stopped iff mode == Email" representable rather than implicit. - Redundant params —
TvSignInTextFieldtakes bothisErroranderrorText, and every call site derives one from the other (TvEmailSignInForm.kt:74,:88).errorText != nullis sufficient. - Preview coverage regressed —
mainhad previews for the sign-in loading and error states; the new file only previews QR-Readyand empty-email. Missing: QR loading, QR error, email with inline errors, email submitting. The checklist claims preview coverage, and it's the error/submitting states that most need eyeballs here. - Duplicate label — the tab reads "Email" (
tv_sign_in_tab_email) and so does the field placeholder directly beneath it (profile_email). tvOS uses "User-Pass"; something like "Email & password" would disambiguate. - String naming —
profile_create_password_requirements("Password must be at least 6 characters") is an account-creation string reused on a login form. The text is accurate, so this is cosmetic. tv_onboarding_sign_in_titleis now unreferenced invalues/while 20+ translations remain. Deliberate per the description, and I agree with avoiding GlotPress churn — just flagging in case lint'sUnusedResourcespicks it up.
Not verified
I couldn't run ./gradlew :tv:testDebugUnitTest or spotlessCheck — Gradle isn't in this job's allowed tools, so all findings above are from reading the code. Every colour value cited was read from TvColorScheme.kt, and the phone-parity comparisons from OnboardingLogInViewModel / AccountViewModel.
No changes were pushed — this is review feedback only.
• branch feat/tv-manual-sign-in
… visible pill track, keyboard no longer shifts the screen
|
Claude finished @sztomek's task in 7m 0s —— View job Code review:
|
|
Claude finished @sztomek's task in 4m 37s —— View job Code review:
|
| BasicTextField( | ||
| value = value, | ||
| onValueChange = onValueChange, | ||
| enabled = enabled, |
There was a problem hiding this comment.
Thanks for picking up the parity fix — one caveat specific to TV though: enabled = false on BasicTextField makes the field non-focusable, not merely non-editable (CoreTextField gates its Modifier.focusable on enabled).
So on the IME-onDone submit path — focus is on the password field, onSubmit() flips isSubmitting to true — the currently focused node is deactivated mid-flight and Compose clears focus. On a phone that's invisible; on a D-pad-only device it means there is no focused element while the request is in flight, and if the login fails the user has to blind-press the remote to get focus back onto something before they can retry. There's no visible focus ring anywhere to tell them where they are.
readOnly gives you the same "can't edit mid-request" guarantee without touching focusability:
| enabled = enabled, | |
| readOnly = readOnly, |
with readOnly: Boolean = false on the parameter list and readOnly = state.isSubmitting at both call sites (lines 75 and 89). If you'd rather keep enabled, then move focus to the Log in button before disabling, so focus is never dropped.
Either way this is worth exercising on-device: submit from the IME Done key (not the button) with wrong credentials, and check the remote still works afterwards.
|
@geekygecko thanks for the feedback, i've addressed these ui hiccups: pc_tv_signin_demo.mp4 |
# Conflicts: # modules/services/localization/src/main/res/values/strings.xml # tv/src/main/java/au/com/shiftyjelly/pocketcasts/onboarding/signin/TvSignInScreen.kt # tv/src/main/java/au/com/shiftyjelly/pocketcasts/onboarding/signin/TvSignInViewModel.kt # tv/src/test/java/au/com/shiftyjelly/pocketcasts/onboarding/TvSignInViewModelTest.kt
|
Claude finished @sztomek's task in 5m 7s —— View job Code review: pass 4 (
|
|
Claude finished @sztomek's task in 1m 16s —— View job Code review: pass 5 (
|
# Conflicts: # modules/services/localization/src/main/res/values/strings.xml # tv/src/main/java/au/com/shiftyjelly/pocketcasts/onboarding/signin/TvSignInScreen.kt
|
Claude finished @sztomek's task in 2m 3s —— View job Code review: pass 6 (
|
|
Claude finished @sztomek's task in 1m 8s —— View job Code review: pass 7 (
|

Description
Adds a manual email/password sign-in option to the Android TV sign-in screen, which was previously QR-only. This closes an Apple TV parity gap found in an Android-TV-vs-iOS-tvOS onboarding audit: on tvOS the sign-in screen offers a segmented QR / User-Pass toggle, but on Android TV a user without their phone handy had no way to log in at all.
A
QR code/Emailsegmented toggle (tv-material3TabRow+PillIndicator, matchingTvSearchFilters) is added toTvSignInScreen. The QR flow is unchanged. Selecting Email shows an email field, a password field and a Log in button that reuses the existingSyncManager.loginWithEmailAndPassword(...)machinery; on success it navigates to the syncing screen exactly like the QR path.Notes:
user_signed_in/user_signin_failed,source = password) are emitted centrally bySyncManagerImpl(viaLoginIdentity.PocketCasts) — the screen does not double-track them. Only the newsign_in_type_tappedevent is fired from the ViewModel when the toggle is tapped.tv_onboarding_sign_in_title) is no longer shown now that the toggle/QR layout is always present; the string is left in place to avoid GlotPress translation churn.Fixes POC-865 https://linear.app/a8c/issue/POC-865/manual-sign-in
Testing Instructions
Screenshots or Screencast
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xmlsign_in_type_tappedalready exists in the schema; no new events added.)