Skip to content

[TV] Add email/password sign-in option - #5782

Merged
sztomek merged 12 commits into
mainfrom
feat/tv-manual-sign-in
Aug 27, 2026
Merged

sztomek merged 12 commits into
mainfrom
feat/tv-manual-sign-in

Conversation

@sztomek

@sztomek sztomek commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

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 / Email segmented toggle (tv-material3 TabRow + PillIndicator, matching TvSearchFilters) is added to TvSignInScreen. The QR flow is unchanged. Selecting Email shows an email field, a password field and a Log in button that reuses the existing SyncManager.loginWithEmailAndPassword(...) machinery; on success it navigates to the syncing screen exactly like the QR path.

Notes:

  • Sign-in success/failure analytics (user_signed_in / user_signin_failed, source = password) are emitted centrally by SyncManagerImpl (via LoginIdentity.PocketCasts) — the screen does not double-track them. Only the new sign_in_type_tapped event is fired from the ViewModel when the toggle is tapped.
  • Field validation mirrors the phone login (valid email + password ≥ 6 chars) with inline errors; a bad login shows the server's localized error message.
  • The mode toggle selects on click (not focus) so merely moving focus onto the QR tab doesn't mint a new device code or over-count the "tapped" event.
  • The former "Sign in with your phone" loading title (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

  1. Launch the Android TV app while signed out.
  2. From Welcome, select Sign in.
  3. Confirm the QR code tab is selected by default and the QR flow works as before.
  4. Move focus to the Email tab and press OK — the email/password form appears.
  5. Press Log in with empty/invalid fields → inline validation errors appear; no network call is made.
  6. Enter valid credentials and Log in → button shows a spinner, then the app proceeds to the syncing screen and into Home.
  7. Enter wrong credentials → the server error message appears inline and you can retry.

Screenshots or Screencast

Email login Form validation
Screenshot_20260826_113142 Screenshot_20260826_113157

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • 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. (sign_in_type_tapped already exists in the schema; no new events added.)

@dangermattic

dangermattic commented Aug 26, 2026 •

Copy link
Copy Markdown
Collaborator
1 Warning
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@claude

claude Bot commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @sztomek's task in 9m 29s —— View job


Code review: [TV] Add email/password sign-in option

  • Gather context (read diff + changed files)
  • Review ViewModel logic & analytics
  • Review Compose UI (TvSignInScreen, TvEmailSignInForm)
  • Review tests & strings
  • Post review feedback (11 inline comments)

This is a well-structured PR — the QR path is genuinely untouched, SyncManagerImpl is correctly left to own the user_signed_in / user_signin_failed analytics, the double-submit guard is real and tested, and the new tests cover the validation and failure branches. The design decision to select on click rather than focus is the right call for avoiding device-code churn and analytics over-counting.

That decision, though, is also where the main problems are: TvSignInModeTabs is a near-verbatim copy of TvSearchFilters, whose colour scheme assumes focus and selection are the same tab. Decoupling them leaves the segmented control without working visuals.

Blocking

1. Segmented control has no usable focus/selection visuals — TvSignInScreen.kt:175-181, :142-160

Three distinct states are broken, all traceable to reusing TvSearchFilters' colours with onFocus = {}:

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 than Patterns.EMAIL_ADDRESS, so a handful of addresses that work on the phone are rejected on TV. If the regex exists to dodge android.util.Patterns in 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 writes Complete into what is otherwise the QR device-auth state machine. AGENTS.md asks for a single immutable state; one TvSignInUiState carrying mode would make "polling is stopped iff mode == Email" representable rather than implicit.
  • Redundant params — TvSignInTextField takes both isError and errorText, and every call site derives one from the other (TvEmailSignInForm.kt:74, :88). errorText != null is sufficient.
  • Preview coverage regressed — main had previews for the sign-in loading and error states; the new file only previews QR-Ready and 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_title is now unreferenced in values/ while 20+ translations remain. Deliberate per the description, and I agree with avoiding GlotPress churn — just flagging in case lint's UnusedResources picks 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

@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 09:35
@sztomek
sztomek requested a review from a team as a code owner August 26, 2026 09:35
@sztomek
sztomek requested review from geekygecko and removed request for a team August 26, 2026 09:35
@claude

claude Bot commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review: [TV] Add email/password sign-in option

  • Diff the current head (a8e63e5) against origin/main
  • Re-check the items from my earlier pass
  • Review the new manifest / focus-select / centring changes
  • Post inline comments (9)
  • Summarise

Re-reviewed at a8e63e5. Four of my earlier findings are genuinely fixed, and the fixes are the right ones: selectMode no longer calls requestDeviceCode() (so a displayed pairing code survives a trip to the Email tab and back), LoadingView is .size(48.dp), the pill colours now match TvSearchFilters exactly, and the serverError slot reserves 24dp so the form no longer reflows when an error appears.

The QR path is still genuinely untouched, SyncManagerImpl is correctly left to own user_signed_in / user_signin_failed, and the double-submit guard is real and tested.

Blocking

1. The keyboard is shown but never hidden — and adjustNothing removes the fallback — TvEmailSignInForm.kt:171-176, AndroidManifest.xml:48

keyboardController?.show() on focus gain has no matching hide() — not on focus loss, not in onDone. TvSearchField, the existing text-input precedent in this same module, does hide it (TvSearchField.kt:79). Commit 5cee0b6 addressed the layout shift by adding android:windowSoftInputMode="adjustNothing" to TvActivity, which means the IME now stays up and the content underneath can no longer pan out from under it. The Log in button, the reserved server-error slot and the password field's inline error all sit below the email field — i.e. in the region a bottom-anchored leanback IME occupies. That's the exact feedback this PR adds, hidden at the exact moment it appears.

Separately, adjustNothing on TvActivity is app-wide (single-activity app), so it also applies to the search field and every future input surface. Worth scoping to this screen.

2. isSubmitting and the credentials survive a successful login; the poll isn't cancelled — TvSignInViewModel.kt:78-85

isSubmitting is only ever reset on failure, so the button is correct purely because Complete navigates away. The plaintext password stays in a data class whose generated toString() renders it. And now that selectMode (rightly) no longer cancels pollingJob, the device-auth poll runs through the whole email login and past its success — loginWithDeviceAuth calls syncAccountManager.addAccount(...) on success, so a late-resolving poll can overwrite the account that was just signed in and double-fire user_signed_in. Narrow, but there's no reason for two live paths to Complete.

Should fix

3. Email still isn't trimmed — TvSignInViewModel.kt:51. The phone does email.trim(); the anchored regex rejects a trailing space, which leanback and voice input produce routinely and which is invisible from the sofa.

4. sign_in_type_tapped now fires on focus traversal — TvSignInViewModel.kt:41-48. 5cee0b6 went back to onFocus = { onSelect(tabMode) }, so nudging the D-pad left/right emits a "tapped" event each way. The PR description is now stale on this — it still says the toggle selects on click specifically to avoid over-counting, which was true at 66859a4 but not at head. Either the description or the tracking should change.

5. The header and toggle shift when you switch tabs — TvSignInScreen.kt:99-124. The centred Column changes height between the QR branch (heightIn(min = 280.dp)) and the ~230dp email form, so the logo, title and tab row move ~25dp as the mode changes — and with focus-select restored, that happens while focus is crossing the toggle. A fixed-height content area anchors it.

6. Retry lost its auto-focus — TvSignInScreen.kt:225-247. main focused Retry via a FocusRequester; the tab row's LaunchedEffect(Unit) now claims initial focus instead. Also worth confirming on-device that Down from TabRow traverses into the button.

7. Form stays editable during submit; no offline handling — TvEmailSignInForm.kt:116-137. The phone gates fields on enableSubmissionFields and pre-checks Network.isConnected for log_in_no_network. Neither is carried over — so an error can end up attached to credentials no longer on screen. Also: no password reveal affordance, which matters far more with a D-pad keyboard than on a phone.

Nits

  • TvSignInModeTabs is now a line-for-line duplicate of TvSearchFilterPills — TvSignInScreen.kt:128-196. They already differ in inactiveColor for no design reason; extracting a shared TvPillTabs would stop the drift.
  • Fourth copy of credential validation — TvSignInViewModel.kt:100-107. Stricter than Patterns.EMAIL_ADDRESS; please note why the regex is hand-rolled so nobody reverts it to Patterns and breaks the JVM tests.
  • Test helper naming and gaps — TvSignInViewModelTest.kt:262-265. createEmailViewModel() actually disables QR by throwing; and selectMode's error-clearing plus updateEmail's serverError reset are untested.
  • Off-theme colours in TvEmailSignInForm — the focused field uses a hardcoded Color.White (line 154) rather than tvColors.backgroundActive, and the two error texts use MaterialTheme.colorScheme.error, which is tv-material3's darkColorScheme default rather than a Pocket Casts token (TvColorScheme has no error colour). Worth checking that red against backgroundSunken and adding a token if it's wrong.
  • Preview coverage regressed — main had previews for the sign-in loading and error states; this file previews only QR-Ready and email-with-an-address. Missing: QR loading, QR error, email with inline validation errors, email submitting. The checklist claims preview coverage, and the error/submitting states are the ones that most need eyeballs.
  • State modelling — the screen collects three interdependent flows (uiState, mode, emailState) and the email path writes Complete into what is otherwise the QR device-auth state machine. AGENTS.md asks for a single immutable state; one TvSignInUiState carrying mode would say what's true in one place.
  • Redundant/duplicated labels — the tab reads "Email" and the placeholder directly beneath it also reads "Email" (profile_email); tvOS uses "User-Pass". And profile_create_password_requirements is an account-creation string reused as a login validation message (accurate text, cosmetic naming).
  • tv_onboarding_sign_in_title is now unreferenced from Kotlin while the base value and 20+ translations remain. Deliberate per the description and I agree with avoiding GlotPress churn — just noting UnusedResources may warn (won't fail the build; it's a warning and warningsAsErrors isn't set).

Not verified

Gradle isn't in this job's allowed tools, so I could not run :tv:testDebugUnitTest, spotlessCheck or a compile — every finding above comes from reading the code. Colour values were read from TvColorScheme.kt and TvTheme.kt, the phone-parity comparisons from OnboardingLogInViewModel / AccountViewModel, and the account-overwrite claim from SyncManagerImpl.loginWithDeviceAuth. The IME-occlusion consequence in (1) depends on the specific leanback IME's geometry, though the missing hide() is unambiguous.

No changes were pushed — review feedback only.
• branch feat/tv-manual-sign-in

Comment thread tv/src/main/AndroidManifest.xml
@claude

claude Bot commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @sztomek's task in 4m 37s —— View job


Code review: [TV] Add email/password sign-in option — pass 3 (22f16d0)

  • Gather context (new commits since last review)
  • Re-check previously raised findings
  • Review current ViewModel / Compose / tests state
  • Post review feedback (4 inline comments)

22f16d0 cleared most of the last round: keyboard hide() on focus loss and on onDone, moveFocus's result propagated instead of a blanket true, email.trim(), credentials cleared and isSubmitting reset on success, a redacting toString(), Retry's auto-focus restored, LoadingView pinned to 48dp, the test helper renamed and five new tests added. Good turnaround.

One of those fixes introduced a new bug, though, and it's the one I'd hold the merge on.

Blocking

1. A failed email login permanently breaks the QR tab — TvSignInViewModel.kt:72

pollingJob?.cancel() landed before viewModelScope.launch rather than inside the Success branch. On the failure path the poll is never restarted, and selectMode(QrCode) no longer re-requests a device code (correctly — that was the churn fix). So:

Email tab → wrong password → back to QR code tab → the previous user code is still on screen, with nothing polling.

The user enters that code on their phone and the TV sits there forever. There's no escape hatch either: cancelling a Flow collection emits nothing, so deviceAuthFlow never reaches its emit(TvSignInUiState.Error) and the Retry button never composes. _uiState stays Ready. Only backing out of the whole screen recovers.

Moving the cancel into the Success branch keeps the property you wanted (the device poll can't addAccount behind a completed email login) without killing a QR session the user may still be mid-way through.

Should fix

2. enabled = false on BasicTextField drops focus, not just editing — TvEmailSignInForm.kt:171. CoreTextField gates its Modifier.focusable on enabled, so submitting from the IME Done key deactivates the node that currently holds focus. On a D-pad-only device that leaves nothing focused for the duration of the request, and no focus ring to find after a failure. readOnly = state.isSubmitting gives the same guarantee without touching focusability.

3. sign_in_type_tapped fires on focus traversal — TvSignInScreen.kt:162. Unchanged since 5cee0b6. Every D-pad Left/Right across the toggle emits an event; onClick is unreachable as a tracking path (tv Tab only clicks when already focused, and the _mode.value == mode guard then short-circuits). The PR description still describes the 66859a4 click-select behaviour, so it's now stale on this point too.

4. Header and toggle shift when the mode changes — TvSignInScreen.kt:114. QR is pinned to 280dp, the email form measures ~232dp (~275dp once both inline errors render); the centred Column splits the difference, so the logo/title/tabs slide ~24dp — while focus is crossing the toggle, given focus-select.

5. adjustNothing is still app-wide — AndroidManifest.xml:48. TvActivity is the only activity, so this also changes TvSearchField and every future input surface. Less dangerous now that hide() works, but nothing in the test plan covers search. Scoping it to TvSignInScreen via a DisposableEffect on window.setSoftInputMode keeps the blast radius to the feature that needs it.

6. No offline pre-check. The phone shows LR.string.log_in_no_network from a Network.isConnected check; here an offline attempt spins and then surfaces whatever the sync layer produces.

Nits (all carried over, none new)

  • TvSignInModeTabs is now a line-for-line duplicate of TvSearchFilterPills apart from the track colour, inactiveColor and 24 vs 21dp padding — plus the new LaunchedEffect auto-focus. Worth extracting a shared TvPillTabs before a third caller appears.
  • No comment on EMAIL_REGEX explaining why it isn't Patterns.EMAIL_ADDRESS. It's stricter than the phone's (no !#$&'*/=?^\{|}~` in the local part), so a few addresses that log in on mobile are rejected here.
  • Preview coverage is still QR-Ready + empty-email only; the states that most need eyes (email with inline errors, email submitting, QR error) have none.
  • Password field has no reveal affordance — blind-typing on a leanback keyboard is materially harder than on a phone.
  • The tab says "Email" and the placeholder directly under it says "Email" too. tvOS uses "User-Pass".
  • tv_onboarding_sign_in_title is now unreferenced in values/ with 20+ translations still present — deliberate per the description, just watch for lint UnusedResources.
  • profile_create_password_requirements (a creation string) reused as a login error. Text is accurate.

Test suite

The new tests are real ones — trimming, error clearing, cleared credentials, the double-submit guard. Two gaps left: switching back to qr mode tracks qr sign in type asserts against a QR flow that can never reach Ready (it would keep passing if the poll restart broke — verify(syncManager, times(2)).deviceAuthorize() would not), and nothing covers the poll surviving a mode switch, which is exactly the invariant finding #1 breaks.

Not verified

I can't run ./gradlew :tv:testDebugUnitTest or spotlessCheck — Gradle isn't in this job's allowed tools, so everything above comes from reading the code. Finding #1 is traced through TvDeviceAuth.kt and TvSignInViewModel.kt and I'm confident in it; #2 rests on CoreTextField gating focusable on enabled, which is worth a quick on-device check with the IME Done key rather than the button.

No changes were pushed — review feedback only.
• branch feat/tv-manual-sign-in

BasicTextField(
value = value,
onValueChange = onValueChange,
enabled = enabled,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

Suggested change
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.

Fix this →

@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.

Looks great.

I only found a couple of minor issues.

  1. Tapping the auto complete doesn't seem to take the value.
Screen_recording_20260827_152302.mp4
  1. When typing the password the keyboard covers the input box.
Image

@sztomek

sztomek commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

@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

claude Bot commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review: pass 4 (8be8bce)

The pass-3 blocker is fixed the right way — pollingJob?.cancel() now lives in the Success branch, so a failed email login no longer leaves the QR tab showing a stale code with nothing polling. The autofill contentType matches the phone's EmailAndPasswordFields pattern exactly, and the scroll + bringIntoView workaround is a reasonable answer to the IME occlusion given adjustNothing. Top-aligning the column also resolves the header/toggle shift I raised in passes 2–3. Nothing new blocks.

Non-blocking

  • tv/…/signin/TvEmailSignInForm.kt:206 — the 250 ms bringIntoView is launched on rememberCoroutineScope, so it still fires after focus has left the field; key it on isFocused instead. thread
  • tv/src/test/java/…/TvSignInViewModelTest.kt — no test covers the invariant this commit restores: after a failed email login, a subsequent device-auth poll success still reaches Complete. That's exactly the regression 22f16d0 shipped.

Nits

  • tv/…/signin/TvEmailSignInForm.kt:263 — TvSignInKeyboardInset = 320.dp is a guess; prefer WindowInsets.ime with this as the fallback. thread

Still open from earlier passes

  • enabled = !isSubmitting on BasicTextField drops focus rather than just editing; readOnly avoids it — r3861525520
  • sign_in_type_tapped fires on D-pad focus traversal, and the PR description still describes click-select — r3861526973
  • windowSoftInputMode="adjustNothing" is still app-wide on the single TvActivity, so it also applies to search — r3861466977
  • No Network.isConnected pre-check / log_in_no_network parity with the phone — r3861480226

Findings come from reading the diff; Gradle isn't in this job's allowed tools, so I ran no build or tests. The scroll geometry above assumes the 960×540 dp viewport a 1080p TV reports.
• branch feat/tv-manual-sign-in

@claude

claude Bot commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review: pass 5 (89756b9)

Two new commits since my last pass: a merge of origin/main and one typography change. The merge integrates main's sign-in changes cleanly — deviceAuthFlow(syncManager, isNewAccount = false), trackShown() / SignInShownEvent, and the loginWithDeviceAuth arity update in the tests all line up, and the two strings main deleted (tv_create_account_subtitle, tv_create_account_come_back) have no remaining references. 89756b9 is cosmetic only (title1 → title2, headline → body) and now makes every onboarding screen title consistent at title2. Nothing new blocks.

Still open from earlier passes

  • tv/…/signin/TvEmailSignInForm.kt:87,102 — enabled = !isSubmitting on BasicTextField drops focus rather than just editing; readOnly avoids it — r3861525520
  • tv/…/signin/TvSignInScreen.kt:173 — sign_in_type_tapped fires on D-pad focus traversal, and the PR description still describes click-select — r3861526973
  • tv/src/main/AndroidManifest.xml:48 — windowSoftInputMode="adjustNothing" is still app-wide on the single TvActivity, so it also applies to search — r3861466977
  • tv/…/signin/TvEmailSignInForm.kt:179 — the 250 ms bringIntoView still runs on rememberCoroutineScope, so it fires after focus has left the field — r3871099683
  • No Network.isConnected pre-check / log_in_no_network parity with the phone — r3861480226
  • TvSignInViewModelTest.kt — still no test that a device-auth poll success reaches Complete after a failed email login, the invariant 22f16d0 briefly broke.

Findings come from reading the diff and the merged files; Gradle isn't in this job's allowed tools, so I ran no build or tests.
• branch feat/tv-manual-sign-in

# Conflicts:
#	modules/services/localization/src/main/res/values/strings.xml
#	tv/src/main/java/au/com/shiftyjelly/pocketcasts/onboarding/signin/TvSignInScreen.kt
@claude

claude Bot commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review: pass 6 (5c52a9d)

Only new work since pass 5 is the merge of origin/main (which brings in #5788's onboarding copy changes). The merge resolves cleanly everywhere except the shared error composable: main's #5788 refactored TvSignInError into an internal TvSignInErrorContent reused by the create-account modal, and this branch's resolution kept the new body but dropped the trailing LaunchedEffect that requested focus. That silently changes a screen outside this PR's scope.

Blocking

  • tv/…/signin/TvSignInScreen.kt:242 — TvSignInErrorContent builds a FocusRequester and attaches it to Try again but never calls requestFocus(); main does (main:156-158). Because [TV] Align onboarding copies with Apple TV #5788 made this composable shared, TvCreateAccountModal.ErrorContent (TvCreateAccountModal.kt:125) now has no focusable element at all in its error state, while its Ready and Loading states both request focus — so the D-pad does nothing and Try again is unreachable. thread

Still open from earlier passes

  • tv/…/signin/TvEmailSignInForm.kt:87,102 — enabled = !isSubmitting on BasicTextField drops focus rather than just editing — r3861525520
  • tv/…/signin/TvSignInScreen.kt:174 — sign_in_type_tapped fires on D-pad focus traversal; the PR description still describes click-select — r3861526973
  • tv/src/main/AndroidManifest.xml:48 — windowSoftInputMode="adjustNothing" is still app-wide on the single TvActivity — r3861466977
  • tv/…/signin/TvEmailSignInForm.kt:179 — the 250 ms bringIntoView still runs on rememberCoroutineScope, so it fires after focus has left the field — r3871099683
  • No Network.isConnected pre-check / log_in_no_network parity with the phone — r3861480226
  • TvSignInViewModelTest.kt — still no test that a device-auth poll success reaches Complete after a failed email login.

Findings come from reading the merge diff and the merged files against origin/main; Gradle isn't in this job's allowed tools, so I ran no build or tests. No changes were pushed.
• branch feat/tv-manual-sign-in

@claude

claude Bot commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review: pass 7 (cc30071)

One commit since my last pass, and it closes the pass-6 blocker: TvSignInErrorContent gets its trailing LaunchedEffect { focusRequester.requestFocus() } back (TvSignInScreen.kt:271), so Try again is reachable again in both this screen and the shared TvCreateAccountModal error state. The bringIntoView reveal also moved to a LaunchedEffect(isFocused), which cancels it when focus leaves the field during the 250 ms window. Nothing new blocks.

Still open from earlier passes

  • tv/…/signin/TvEmailSignInForm.kt:86,101 — enabled = !isSubmitting on BasicTextField drops focus rather than just editing; readOnly avoids it — r3861525520
  • tv/…/signin/TvSignInScreen.kt:174 — sign_in_type_tapped fires on D-pad focus traversal, and the PR description still describes click-select — r3861526973
  • tv/src/main/AndroidManifest.xml:48 — windowSoftInputMode="adjustNothing" is still app-wide on the single TvActivity, so it also applies to search — r3861466977
  • No Network.isConnected pre-check / log_in_no_network parity with the phone — r3861480226
  • TvSignInViewModelTest.kt — still no test that a device-auth poll success reaches Complete after a failed email login, the invariant 22f16d0 briefly broke.

Findings come from reading the diff and the current files; Gradle isn't in this job's allowed tools, so I ran no build or tests. No changes were pushed.
• branch feat/tv-manual-sign-in

@sztomek
sztomek merged commit b163525 into main Aug 27, 2026
19 checks passed
@sztomek
sztomek deleted the feat/tv-manual-sign-in branch August 27, 2026 11:25
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.

3 participants