feat(tv): account settings on the shared controllers - #170
Conversation
Replace the Account placeholder with the oracle's settings pane: identity and quota header with Sign out, Playback settings (proxy picker, resume, playback type, autoplay, subtitles), Storage settings (Trash on or off with confirmation, Manage your trash with its size, history) and App and device information with a Diagnostics dialog, Sign out as the final row. Switches and choice dialogs save through the shared AccountSettingsController and AndroidAppConfigController held by the TV session; a 401 from either, or from the route list, rejects the session, and the History pane follows the confirmed history setting. TvChoiceDialog is the shared centred choice dialog (Sort delegates to it), TvShell takes an accountPane slot, and the avatar URL policy moves to main so TV can load avatars through coil.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Live proof on the API 36 TV emulator,
Remember current place in video files toggled true → false → true; each state read back with |
There was a problem hiding this comment.
🟡 Changes recommended
TvChoiceDialog can crash if instantiated with an empty choices list due to unconditional focus/bring-into-view requests.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Implements the full Android TV Account/Settings pane to match the behavior oracle and TV parity contract, wiring it to the shared account-wide settings (/account/settings) and app config (/config) controllers owned by TvSession.
Changes:
- Adds
TvAccountScreenwith identity/quota header, grouped settings rows, centered choice dialogs, and Diagnostics, plus Trash round-trip behavior. - Introduces a shared centered
TvChoiceDialogand refactorsTvSortDialogto delegate to it. - Moves avatar URL validation to shared
maincode and adds TV-specific resources (strings, Phosphor icons), plus session/controller wiring and tests.
File summaries
| File | Description |
|---|---|
| README.md | Updates TV feature status to include the new Account/settings pane behavior. |
| docs/harness.md | Documents TV Account proof steps and expected behaviors for oracle captures 09–12 and 14. |
| design/phosphor-icons.lock.json | Adds newly generated Phosphor icon entries for TV resources. |
| app/src/tv/res/values/strings.xml | Adds TV Account/settings strings (sections, rows, dialogs, diagnostics). |
| app/src/tv/res/drawable/ic_ph_tree_structure.xml | New generated TV icon. |
| app/src/tv/res/drawable/ic_ph_television.xml | New generated TV icon. |
| app/src/tv/res/drawable/ic_ph_subtitles.xml | New generated TV icon. |
| app/src/tv/res/drawable/ic_ph_sign_out.xml | New generated TV icon. |
| app/src/tv/res/drawable/ic_ph_recycle.xml | New generated TV icon. |
| app/src/tv/res/drawable/ic_ph_monitor_play.xml | New generated TV icon. |
| app/src/tv/res/drawable/ic_ph_list_checks.xml | New generated TV icon. |
| app/src/tv/res/drawable/ic_ph_circles_four.xml | New generated TV icon. |
| app/src/tv/res/drawable/ic_ph_bug.xml | New generated TV icon. |
| app/src/tv/res/drawable/ic_ph_bookmark_simple.xml | New generated TV icon. |
| app/src/tv/res/drawable/ic_ph_android_logo.xml | New generated TV icon. |
| app/src/tv/kotlin/io/putdotio/android/tv/TvShell.kt | Replaces trashPane with an accountPane slot and exposes full-width focused scale. |
| app/src/tv/kotlin/io/putdotio/android/tv/TvSessionViewModel.kt | Adds settings/app-config controllers to TvSession lifecycle and dependencies. |
| app/src/tv/kotlin/io/putdotio/android/tv/TvPaneFocusOwner.kt | Adds claim() to support controlled entry focus behavior. |
| app/src/tv/kotlin/io/putdotio/android/tv/TvChoiceDialog.kt | Adds a shared centered choice dialog for TV pickers. |
| app/src/tv/kotlin/io/putdotio/android/tv/files/TvSortDialog.kt | Refactors sort dialog to use TvChoiceDialog. |
| app/src/tv/kotlin/io/putdotio/android/tv/auth/TvSessionGateway.kt | Extends TvAccount with avatarUrl and maps it from SDK account info. |
| app/src/tv/kotlin/io/putdotio/android/tv/account/TvAccountStrings.kt | Adds TV string mappings for settings/app-config failures and playback type labels. |
| app/src/tv/kotlin/io/putdotio/android/tv/account/TvAccountScreen.kt | Implements the full TV Account/settings UI including focus rules, dialogs, and trash swap. |
| app/src/tv/kotlin/io/putdotio/android/tv/account/TvAccountDialogs.kt | Adds proxy picker, trash disable confirmation, and diagnostics dialog implementations. |
| app/src/tv/kotlin/io/putdotio/android/PutioApp.kt | Wires TV Account pane into the signed-in shell and session rejection conditions. |
| app/src/testTv/kotlin/io/putdotio/android/TvShellTest.kt | Updates shell test to exercise Account → Trash → Back focus behavior with the new screen. |
| app/src/testTv/kotlin/io/putdotio/android/tv/TvSessionViewModelTest.kt | Extends session tests with stub settings/app-config repositories. |
| app/src/testTv/kotlin/io/putdotio/android/tv/auth/PutioTvSessionGatewayTest.kt | Updates gateway test expectations for avatarUrl. |
| app/src/testTv/kotlin/io/putdotio/android/tv/account/TvAccountScreenTest.kt | Adds focused UI tests for TV Account screen behaviors (dialogs, toggles, retry, focus). |
| app/src/mobile/kotlin/io/putdotio/android/MobileAccountScreen.kt | Removes duplicated avatar URL validation now shared in main. |
| app/src/main/kotlin/io/putdotio/android/AvatarUrl.kt | Adds shared isSupportedAvatarUrl() used by mobile and TV. |
| app/build.gradle.kts | Adds Coil dependencies to the TV source set for avatar loading. |
Review details
- Files reviewed: 32/32 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8512f1468
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Settings arriving while a dialog from another row is up, or while the user sits on another row, no longer reclaim the entry point for Choose your proxy; the claim only replaces the header's Sign out. The choice dialog requests nothing when it has no choices.
There was a problem hiding this comment.
🟡 Changes recommended
There is a focus-stealing edge case in TvAccountScreen where an unconditional initial focus request can pull focus back from the drawer after the user has moved away.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 32/32 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce1e8ccba6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…nfo rows out of focus A failed save shows under the row it belongs to and Try again hands focus back to that row when the retry removes the notice; Manage your trash stays reachable when account settings fail to load; App, Device and Operating system are facts, not controls, so the D-pad skips them; the first-visit focus request yields when the drawer has since taken focus.
There was a problem hiding this comment.
🔵 Needs a closer look
The change introduces a large new TV screen with intricate focus/state behavior and new session-owned controllers that should be validated with final human QA on a real TV/emulator flow beyond unit tests.
Review details
- Files reviewed: 32/32 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38ac91f01d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
One saveable dialog state names the row that opened it, so a dialog restored with the pane hands focus back to that row rather than the first one. The History pane follows confirmed settings values only; an unsettled History write leaves the last value in place, as on mobile.
There was a problem hiding this comment.
🟡 Changes recommended
TvAccountScreen’s dialog/focus restoration can treat a dialog as open when its prerequisites aren’t ready, leaving an invalid/unattached focus target after recreation.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 32/32 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04070aef5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A Route or PlaybackType dialog restored before settings or app config are ready is not up until they are, so no focus bookkeeping points at a row that is not composed yet.
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large new TV settings surface with non-trivial focus/dialog/state interactions across multiple controllers that warrants final human verification.
Review details
- Files reviewed: 32/32 changed files
- Comments generated: 0 new
- Review effort level: Lite
Part of #33 (Account/settings, oracle captures
09–12and14).Problem
The TV Account destination was a placeholder: a "Signed in as" line, Manage your trash, and Sign out. The oracle asks for the identity and quota header, grouped settings rows with M3 switches, centred choice dialogs for proxy and playback type, storage settings with Trash under them, app and device information with a Diagnostics entry, and Logout as the final row.
Solution
TvAccountScreen(app/src/tv/.../tv/account/) renders the pane on the sharedAccountSettingsController(account-wide/account/settings) andAndroidAppConfigController(this app's/config), both now owned byTvSessionand closed with it:main), username, "X of Y free" bar, Sign out button.use_start_from), Video playback type (MP4 above HLS (default), as the oracle lists them), Autoplay next video, Show subtitles, Do not select subtitles by default.confirmedHistoryEnabled()).TvChoiceDialogis the shared centred choice dialog (TvSortDialogdelegates to it) andTvShelltakes anaccountPaneslot. Video playback buffer size stays out: there is no server key for it (see the Settings rollout notes).Proof
./gradlew verify :buildSrc:test :app:assembleTvProductionDebug :app:assembleMobileProductionDebuggreen;TvAccountScreenTest(8),TvShellTestTrash round trip,TvSessionViewModelTestupdated.Live on the API 36 TV emulator with the
devs-autoidentity: the pane loads the account's real values (cdn77, resume on, subtitles on, do-not-autoselect on, trash on, history on); Video playback type HLS → MP4 → HLS saved through/config; Remember current place true → false → true read back withputio sdk call --operation account.getSettings; the proxy picker listed the account's routes; Turn off trash? cancelled and lefttrash_enabled: true; Manage your trash opened Trash and Back returned to that row; Diagnostics opened; Sign out is the last row. Captures follow in a comment.