fix(tv): harden focus foundations across the app - #167
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAndroid TV focus behavior now uses bounded retries, stable identities, explicit control states, and screen-specific restoration. Recommendation section IDs, detail-return focus, Calendar routing, Diagnostics traversal, selector interaction, tests, and Android test dependencies were updated. ChangesAndroid TV focus foundations
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Nine review findings on the focus-foundations work. Focus graph: - Split disabled state into structural (leaves the graph) and transient (stays focusable, action suppressed) via TvControlState. The PIN keypad, join-code grid and admin scan card gate on work in flight; dropping them out of the graph stranded the D-pad, since TV does not re-home focus and every initial-focus policy here is one-shot. - rememberTvDialogInitialFocus falls back to moveFocus(Enter) when its budget runs out, so exhaustion cannot end in a dead overlay. Identity: - Disambiguate recommendation section IDs. The server omits section_kind for row types it does not recognise, so the type+label fallback is a live path and a duplicate LazyColumn key is a crash, not a degraded render. Detail return: - For You keeps FocusRequester.Default as the restorer fallback. It arms at resume, one composition after the shell's synchronous claim, so naming its requester handed the restorer a detached node. - Card disposal clears the attachment latch instead of setting a terminal one: LazyRow disposes on viewport recycling, which was abandoning restores that would have succeeded. - Collapse the per-root restore flags into one detailReturnRoot. Budgets: - Name the two budgets. Acquisition (nothing focused yet, 2.4s) stays long because exhaustion means a dead D-pad; relocation (focus already usable, 480ms) is short because exhaustion degrades to a working fallback. For You and Diagnostics move from 6 frames onto the relocation budget. Minor: - Selector collapse is derived at read time, not deferred to an effect. - nextTvDiagnosticsCrashFocus returns null for a control outside the current order rather than coercing the miss to index 0 and sending Down upwards. - Conflate top-anchor position events; drop the constant targetState argument; rename For You's onItemClick to onSavedListItemClick. - Source-guard tests match whitespace-insensitively so they assert the rule rather than a formatting snapshot. Verification: :androidTvApp:testDebugUnitTest (819) and :shared:testDebugUnitTest (978) pass with 0 failures; both debug APKs assemble. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
androidTvApp/build.gradle.kts (1)
113-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the Compose test dependency to the version catalog.
Line 113 pins
androidx.compose.ui:ui-test-junit4:1.9.2inline, while every other dependency in this block uses alibs.alias. An inline version can drift from the Compose UI version that the app modules resolve, and the Robolectric Compose harness then runs against a different Compose runtime than production code. The same literal appears at Line 237 forui-test-manifest.Declare both coordinates in
gradle/libs.versions.tomlwith the existing Compose version reference, then use the aliases here.♻️ Proposed change
implementation(libs.robolectric) implementation(libs.androidx.test.core) - implementation("androidx.compose.ui:ui-test-junit4:1.9.2") + implementation(libs.androidx.compose.ui.test.junit4)dependencies { coreLibraryDesugaring(libs.desugar.jdk.libs) - debugImplementation("androidx.compose.ui:ui-test-manifest:1.9.2") + debugImplementation(libs.androidx.compose.ui.test.manifest) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidTvApp/build.gradle.kts` at line 113, Move the inline ui-test-junit4 and ui-test-manifest dependencies to gradle/libs.versions.toml, defining them with the existing Compose version reference. Update the dependency declarations in the relevant Gradle block to use their libs aliases, matching the surrounding catalog-based dependencies.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/diagnostics/TvDiagnosticsSettingsScreen.kt (1)
134-153: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBind the crash report focus traversal to the rendered row order.
TvDiagnosticsSettingsScreenrenders crash consent rows by iteratingDiagnosticsConsentMode.entries, buttvDiagnosticsCrashFocusOrder()still hardcodesASK, ALWAYS, NEVER. Move the order construction to deriveTvDiagnosticsCrashFocusvalues fromDiagnosticsConsentMode.entriesso an enum reorder cannot make D-pad Down traverse a different sequence than the UI layout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/diagnostics/TvDiagnosticsSettingsScreen.kt` around lines 134 - 153, Update tvDiagnosticsCrashFocusOrder() to construct its TvDiagnosticsCrashFocus sequence by mapping DiagnosticsConsentMode.entries instead of hardcoding ASK, ALWAYS, NEVER. Keep the focus traversal order aligned with the row iteration in TvDiagnosticsSettingsScreen so D-pad navigation follows the rendered enum order.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvCascadeSelector.kt (1)
86-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove
mutableStateMapOffromlibraryRequestersand prune missing library keys.
stableIdentityValueswritesvaluesById.getOrPut(...)during composition, so passing themutableStateMapOfcreates a backwards Compose snapshot write each time a visible library first appears. The downstream reads only access the map for a cachedFocusRequester, so a normalremember { mutableMapOf<Int, FocusRequester>() }is sufficient. Also remove keys that are no longer in the libraryidsso removed library requesters do not accumulate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvCascadeSelector.kt` around lines 86 - 97, Update the libraryRequester state around stableIdentityValues to use a remembered regular MutableMap instead of mutableStateMapOf, since cached FocusRequester reads do not require observable map state. Before or while resolving the current library ids, prune entries whose keys are absent from those ids, then pass the map to stableIdentityValues while preserving stable requester reuse and display ordering.androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvControlSemanticsTest.kt (1)
29-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRequest focus in the transient-control test.
Focused = falseonly verifies the initial state. It does not verify that the transient control accepts focus. Add aFocusRequester, request focus aftersetContent, and assertassertIsFocused()together withassertIsNotEnabled().Confirm the focus-request assertion against the locked Compose UI test API before merging. As per coding guidelines, “Add focused tests for shared logic only when behavior is critical or high risk.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvControlSemanticsTest.kt` around lines 29 - 44, The transient-control test only verifies its initial unfocused state; update transientlyDisabledControlRemainsFocusableAndReportsDisabled to create and attach a FocusRequester, request focus after setContent, then assert the control is focused while retaining assertIsNotEnabled(). Use the locked Compose UI test API for the focus request and focused assertion.Source: Coding guidelines
docs/superpowers/specs/2026-08-03-shield-focus-restoration-design.md (1)
44-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the return-target description with the implementation.
The implementation captures the launch card in
onItemClickimmediately before navigation. It does not update the target whenever focus changes. Update this section to describe the launch snapshot and its purpose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-08-03-shield-focus-restoration-design.md` around lines 44 - 47, Update the return-target description to state that the launch card snapshot is captured in onItemClick immediately before navigation, rather than on every focus change. Describe that this snapshot preserves the section/content identities and row/card indices for restoring focus to the card that launched detail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvObservedFocusPolicy.kt`:
- Around line 16-24: Rename the four top-level properties
TvFocusAcquisitionBudgetMillis, TvFocusRelocationBudgetMillis,
TvApproximateFrameMillis, and TvFrameRelocationMaxAttempts to lower-camel-case
names, then update every reference to those constants throughout the codebase
while preserving their values and behavior.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/diagnostics/TvDiagnosticsSettingsScreen.kt`:
- Around line 279-288: Update tvDiagnosticsCrashFocusKeyResult so repeat events
also resolve the neighboring focus via nextTvDiagnosticsCrashFocus, and set
consume only when that target exists. Preserve the non-repeat behavior while
allowing held Up/Down input to advance focus and preventing consumption at
section boundaries.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvJoinCodeDialog.kt`:
- Around line 223-229: Update the join-code key content in TvJoinCodeDialog so
the glyph inherits the state-aware contentColor from
ClickableSurfaceDefaults.colors. Remove the explicit Text color near the
join-code rendering, or derive it from controlState.actionable, ensuring
non-actionable keys use the disabled color while busy.
---
Nitpick comments:
In `@androidTvApp/build.gradle.kts`:
- Line 113: Move the inline ui-test-junit4 and ui-test-manifest dependencies to
gradle/libs.versions.toml, defining them with the existing Compose version
reference. Update the dependency declarations in the relevant Gradle block to
use their libs aliases, matching the surrounding catalog-based dependencies.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvCascadeSelector.kt`:
- Around line 86-97: Update the libraryRequester state around
stableIdentityValues to use a remembered regular MutableMap instead of
mutableStateMapOf, since cached FocusRequester reads do not require observable
map state. Before or while resolving the current library ids, prune entries
whose keys are absent from those ids, then pass the map to stableIdentityValues
while preserving stable requester reuse and display ordering.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/diagnostics/TvDiagnosticsSettingsScreen.kt`:
- Around line 134-153: Update tvDiagnosticsCrashFocusOrder() to construct its
TvDiagnosticsCrashFocus sequence by mapping DiagnosticsConsentMode.entries
instead of hardcoding ASK, ALWAYS, NEVER. Keep the focus traversal order aligned
with the row iteration in TvDiagnosticsSettingsScreen so D-pad navigation
follows the rendered enum order.
In
`@androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvControlSemanticsTest.kt`:
- Around line 29-44: The transient-control test only verifies its initial
unfocused state; update
transientlyDisabledControlRemainsFocusableAndReportsDisabled to create and
attach a FocusRequester, request focus after setContent, then assert the control
is focused while retaining assertIsNotEnabled(). Use the locked Compose UI test
API for the focus request and focused assertion.
In `@docs/superpowers/specs/2026-08-03-shield-focus-restoration-design.md`:
- Around line 44-47: Update the return-target description to state that the
launch card snapshot is captured in onItemClick immediately before navigation,
rather than on every focus change. Describe that this snapshot preserves the
section/content identities and row/card indices for restoring focus to the card
that launched detail.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b9cb1d4-e3d5-46e8-9f41-6562e7918081
📒 Files selected for processing (47)
androidTvApp/build.gradle.ktsandroidTvApp/gradle.lockfileandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvAnchoredSelectorMenu.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvAuroraChrome.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvCascadeSelector.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvDialogInitialFocus.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvMediaRow.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvOptionDialog.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvPinEntryDialog.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvSquaredButtons.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvControlEnablement.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvObservedFocusPolicy.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/admin/TvAdminScansScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/calendar/TvCalendarScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvPlaybackSelectorRow.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/recommendations/TvRecommendationsFocusBridge.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/recommendations/TvRecommendationsScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvCardOverlaySettingsScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/diagnostics/TvDiagnosticsSettingsScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvJoinCodeDialog.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvDetailReturnFocusState.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvMainShell.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/components/TvAnchoredSelectorMenuStateTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/components/TvCascadeSelectorIdentityTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/components/TvDialogInitialFocusTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/components/TvMediaRowFocusRestoreTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvControlEnablementTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvControlSemanticsTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvObservedFocusPolicyTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/calendar/TvCalendarFocusRoutingTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvPlaybackFormattingTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/recommendations/TvRecommendationsFocusBridgeTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/recommendations/TvRecommendationsTopAnchorTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/settings/diagnostics/TvDiagnosticsStateTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/shell/TvDetailReturnFocusStateTest.ktdocs/superpowers/plans/2026-08-03-for-you-late-focus-relocation.mddocs/superpowers/plans/2026-08-03-shield-focus-restoration.mddocs/superpowers/plans/2026-08-04-focus-foundations-enabled-controls.mddocs/superpowers/plans/2026-08-04-pr-164-review-remediation.mddocs/superpowers/specs/2026-08-03-for-you-late-focus-relocation-design.mddocs/superpowers/specs/2026-08-03-shield-focus-restoration-design.mddocs/superpowers/specs/2026-08-04-pr-164-review-remediation-design.mddocs/superpowers/specs/2026-08-04-whole-application-focus-hardening-design.mdgradle/verification-metadata.xmlshared/src/commonMain/kotlin/org/siloserver/silo/model/recommendation/RecommendationModels.ktshared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/RecommendationsViewModel.ktshared/src/commonTest/kotlin/org/siloserver/silo/viewmodel/RecommendationsSectionIdentityTest.kt
| internal const val TvFocusAcquisitionBudgetMillis = 2_400L | ||
| internal const val TvFocusRelocationBudgetMillis = 480L | ||
|
|
||
| /** `withFrameNanos` cadence on a 60 Hz panel, used to size frame-based budgets. */ | ||
| internal const val TvApproximateFrameMillis = 16L | ||
|
|
||
| /** Frame-paced attempts that fit inside [TvFocusRelocationBudgetMillis]. */ | ||
| internal const val TvFrameRelocationMaxAttempts = | ||
| (TvFocusRelocationBudgetMillis / TvApproximateFrameMillis).toInt() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use camelCase for the focus budget properties.
TvFocusAcquisitionBudgetMillis, TvFocusRelocationBudgetMillis, TvApproximateFrameMillis, and TvFrameRelocationMaxAttempts are properties. Rename them with a lowercase initial and update their references.
As per coding guidelines, “Use camelCase for functions and properties.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvObservedFocusPolicy.kt`
around lines 16 - 24, Rename the four top-level properties
TvFocusAcquisitionBudgetMillis, TvFocusRelocationBudgetMillis,
TvApproximateFrameMillis, and TvFrameRelocationMaxAttempts to lower-camel-case
names, then update every reference to those constants throughout the codebase
while preserving their values and behavior.
Source: Coding guidelines
| internal fun tvDiagnosticsCrashFocusKeyResult( | ||
| current: TvDiagnosticsCrashFocus, | ||
| direction: TvDiagnosticsFocusDirection, | ||
| debugLoggingEnabled: Boolean, | ||
| isRepeat: Boolean, | ||
| ): TvDiagnosticsCrashFocusKeyResult { | ||
| if (isRepeat) return TvDiagnosticsCrashFocusKeyResult(target = null, consume = true) | ||
| val target = nextTvDiagnosticsCrashFocus(current, direction, debugLoggingEnabled) | ||
| return TvDiagnosticsCrashFocusKeyResult(target = target, consume = target != null) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Held Up/Down never advances focus.
Line 285 returns consume = true with target = null for every repeat event. onPreviewKeyEvent then swallows the key without moving focus. A user who holds Down on the remote moves one row on the first event and then stops, because all following events carry repeatCount > 0. Compose's default traversal is also blocked, so the section becomes unresponsive until the key is released.
If the intent is only to prevent repeat events from crossing a section boundary, resolve the neighbour on repeats as well and consume only when a neighbour exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/diagnostics/TvDiagnosticsSettingsScreen.kt`
around lines 279 - 288, Update tvDiagnosticsCrashFocusKeyResult so repeat events
also resolve the neighboring focus via nextTvDiagnosticsCrashFocus, and set
consume only when that target exists. Preserve the non-repeat behavior while
allowing held Up/Down input to advance focus and preventing consumption at
section boundaries.
| colors = ClickableSurfaceDefaults.colors( | ||
| containerColor = Color.White.copy(alpha = 0.06f), | ||
| containerColor = if (enabled) { | ||
| Color.White.copy(alpha = 0.06f) | ||
| } else { | ||
| Color.White.copy(alpha = 0.03f) | ||
| }, | ||
| contentColor = if (enabled) Color.White else Color.White.copy(alpha = 0.42f), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the target file and relevant surrounding code.
fd -a 'TvJoinCodeDialog.kt' .
file="$(fd 'TvJoinCodeDialog.kt' . | head -n 1)"
echo "TARGET=$file"
wc -l "$file"
echo "--- outline ---"
ast-grep outline "$file" || true
echo "--- lines 200-285 ---"
sed -n '200,285p' "$file" | nl -ba -v200
echo "--- usages of clickable label color/style in file ---"
rg -n "color =|Clickable|Surface|contentColor|LocalContentColor|actionable|controlState" "$file"Repository: Silo-Server/silo-android
Length of output: 912
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvJoinCodeDialog.kt"
echo "--- lines 190-276 ---"
awk '{printf "%4d %s\n", NR, $0}' "$file" | sed -n '190,276p'
echo "--- focused searches ---"
rg -n "contentColor|LocalContentColor|Text\\(|colors = ClickableSurfaceDefaults|ClickableSurfaceDefaults|actionable|controlState|isBusy|perform\\(" "$file"
echo "--- imports/dependencies containing tv-material/local content color api? ---"
rg -n "ExperimentalTvMaterial3Api|androidx\\.compose\\.material3\\.experimental\\.tv|Surface|ClickableSurfaceDefaults|contentColor|LocalContentColor" androidTvApp/build.gradle.kts $(find androidTvApp -maxdepth 3 \( -name 'build.gradle.kts' -o -name '*.gradle.kts' \) -print) 2>/dev/null || trueRepository: Silo-Server/silo-android
Length of output: 5390
🌐 Web query:
Jetpack TV Material Theme.tv TypographySurface contentColor LocalContentColor tv.material3
💡 Result:
In Jetpack Compose for TV (androidx.tv.material3), MaterialTheme, typography, color handling, and content coloring function similarly to standard Material 3 but are specifically optimized for TV environments [1][2]. MaterialTheme The MaterialTheme composable in androidx.tv.material3 is the entry point for configuring your app's theme [3]. It provides the ColorScheme, Typography, and Shapes to your UI hierarchy [3]. You can use it at the top level of your application or nest it to override theme attributes for specific sections [3]. Typography TV Material3 provides a type scale system defined by the Typography class [4][5]. You access these styles using MaterialTheme.typography [5]. When creating Text components, you can apply these styles via the style parameter [6][5]. If no style is explicitly set, components will default to the value provided by the current LocalTextStyle [6]. Content Coloring (LocalContentColor) LocalContentColor is a CompositionLocal that defines the preferred color for content (such as text or iconography) within a given hierarchy [4]. - It automatically updates based on the background color to ensure readability [4]. - The Text component automatically uses LocalContentColor if no specific color is provided in the Text parameters or the TextStyle [6]. - Generally, components like Surface use ColorScheme to determine the appropriate content color (often using the "on" color counterparts, such as onSurface) [4]. Key Best Practices - Use androidx.tv.material3 exclusively to avoid conflicts with standard Material 3 libraries [1]. - Use Surface and other TV-optimized components to benefit from built-in focus indicators and remote-friendly behavior [1]. - Use contentColorFor(backgroundColor) to programmatically determine the correct contrast color when defining custom surfaces [4].
Citations:
- 1: https://developer.android.com/training/tv/playback/compose
- 2: https://developer.android.com/jetpack/androidx/releases/tv
- 3: https://developer.android.com/reference/kotlin/androidx/tv/material3/MaterialTheme.composable
- 4: https://developer.android.com/reference/kotlin/androidx/tv/material3/package-summary
- 5: https://developer.android.com/develop/ui/compose/designsystems/material3
- 6: https://developer.android.com/reference/kotlin/androidx/tv/material3/Text.composable
🌐 Web query:
androidx tv material3 ClickableSurfaceDefaults colors contentColor Text uses LocalContentColor documentation
💡 Result:
In the androidx.tv.material3 library, the relationship between Surface components, LocalContentColor, and Text is designed to ensure proper color inheritance and hierarchy [1][2]. 1. Surface and LocalContentColor: When you use a Surface (such as a clickable surface configured via ClickableSurfaceDefaults), the component automatically manages its content color based on its state (e.g., enabled, focused, pressed) [3]. The Surface implementation provides the appropriate content color to the hierarchy by using CompositionLocalProvider to update the LocalContentColor [3]. 2. How Text Uses LocalContentColor: The Text component in androidx.tv.material3 is designed to respect this hierarchy [2]. When a Text component is placed within a Surface, it defaults to using the current value of LocalContentColor if no explicit color is provided in its parameters or its TextStyle [2]. Specifically, if the Text color parameter is Color.Unspecified and the provided style does not have a color set, the Text component will use the value found in LocalContentColor [2]. 3. Role of ClickableSurfaceDefaults: ClickableSurfaceDefaults.colors provides the definitions for the content colors for various interaction states (e.g., contentColor, focusedContentColor, pressedContentColor, disabledContentColor) [4][5][6]. These colors are then used by the Surface component to determine what value to provide to LocalContentColor for the underlying content [3]. In summary, Surface sets the LocalContentColor based on its current state (determined by the colors defined in ClickableSurfaceDefaults), and Text automatically consumes this LocalContentColor to ensure the text remains legible against the surface's background [1][3][2].
Citations:
- 1: https://developer.android.com/reference/kotlin/androidx/tv/material3/package-summary
- 2: https://developer.android.com/reference/kotlin/androidx/tv/material3/Text.composable
- 3: https://android.googlesource.com/platform/frameworks/support.git/+/333932c97261c7b9cab1e8d9e15abc2624016a25/tv/tv-material/src/main/java/androidx/tv/material3/Surface.kt
- 4: https://developer.android.com/reference/kotlin/androidx/tv/material3/ClickableSurfaceDefaults
- 5: https://composables.com/jetpack-compose/androidx.tv/tv-material/objects/ClickableSurfaceDefaults/api
- 6: https://developer.android.google.cn/reference/kotlin/androidx/tv/material3/ClickableSurfaceDefaults
Let the join-code glyph inherit the Surface content color.
The explicit Text color at line 272 overrides the state-aware color supplied by ClickableSurfaceDefaults.colors(...), so non-actionable keys remain white while isBusy is true. Remove the explicit color or derive it from controlState.actionable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvJoinCodeDialog.kt`
around lines 223 - 229, Update the join-code key content in TvJoinCodeDialog so
the glyph inherits the state-aware contentColor from
ClickableSurfaceDefaults.colors. Remove the explicit Text color near the
join-code rendering, or derive it from controlState.actionable, ensuring
non-actionable keys use the disabled color while busy.
The two androidx.compose.ui:ui-test-* coordinates were pinned inline at 1.9.2. Catalog them so the Robolectric harness and the app cannot drift onto different androidx.compose.ui versions; the lockfile resolves ui/ui-android to 1.9.2, so the pin matches what ships. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A LazyRow disposes items on ordinary viewport recycling, so "disposed" and "not attached right now" are the same observation from inside the retry loop — and giving up on the former abandoned restores that would have succeeded. Genuine removal is handled a level up: the content id drops out of the feed, so the pending location resolves elsewhere or to null and the effect is cancelled before the loop is entered. The state is now two-valued, and a target that never attaches spends its budget and exhausts without ever requesting focus on an absent node. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Requiring kind AND key pushed the keyless singleton rows onto the legacy path, whose identity includes the row's contents — so "Popular" and "Recently Added" got a new section id on every refresh, which is what the For You detail return matches on. Accepting a bare kind is not safe either: two keyless rows sharing a kind encode identically and toResolvedSections drops duplicates, so a section would silently vanish from the feed. The kind alone is now trusted only for the five kinds the server emits at most once; a repeatable or unrecognised kind arriving without a key falls back to content identity. The allowlist holds the server's hyphenated wire values, not the underscored row types — an existing test asserted on "recently_added", which is not a kind the server ever sends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OverlayTile declared an `enabled` parameter it never used — its Surface
got no `enabled`, and the caller swallowed the press with
`onClick = { if (enabled) ... }`. With the admin kill-switch on, every
tile stayed in D-pad traversal and did nothing: focusable but inert, the
exact anti-pattern this work exists to remove.
`enabled` is server-driven and refreshes on foreground, so it can also
flip while the screen is open. That empties the right pane of focusable
nodes at once, and Android TV does not re-home focus when the focused
node stops being focusable, so the edge now relocates focus to the
preview pane and closes the detail panel.
The call-site guard is rewritten to catch both. Assertions are scoped to
the declaration that owns the wiring and to that primitive's own argument
list, rather than searching whole files — TvCardOverlaySettingsScreen has
six `enabled = enabled` occurrences, so a whole-file search stayed green
while OverlayTile had none. Comments are stripped so prose cannot satisfy
a rule, and the click-swallow rule matches the shape of the guard instead
of the literal `onClick()`, which is how this bug survived the last pass.
Verified red-green: reintroducing the defect fails both tests, and the
substitution attack (strip the primitive's wiring, add a decoy elsewhere
in the same declaration) fails the argument-scoped count.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
Behaviour change worth calling out
selectorIsInteractivenow counts final actionable options rather than raw track counts. Two user-visible consequences:Both are intended; the audio selector still goes inert when the only entry beside Auto is the disabled "Unknown" fallback.
Review remediation (
fc15f3ac)All nine findings from review are fixed.
Focus graph — the disabled propagation had to be split in two. Structural unavailability (a one-choice selector, Debug logging under consent NEVER) correctly leaves the focus graph. In-flight gating must not: Android TV does not re-home focus when the focused node stops being focusable, and every initial-focus policy here is one-shot. The PIN keypad (
!isVerifying), the join-code grid (!isBusy) and the admin scan card (!scanningAll) were all wired the first way, so a rejected PIN or a running scan left nothing focused and only Back working.TvControlState.structural/.transientnames the distinction and the source guard now asserts both directions.rememberTvDialogInitialFocusfalls back tomoveFocus(Enter)on budget exhaustion, so running out of retries can no longer end in the dead overlay the policy exists to prevent.Section identity.
discoverRowSectionKeyreturns an empty kind for row types it does not recognise, and both fields areomitempty— so thetype=…:label=…fallback is a live path, including for any server predatingsection_kind. Section IDs key aLazyColumn, where a duplicate is anIllegalArgumentExceptionrather than a degraded render. IDs are now disambiguated on collision.Detail return. For You keeps
FocusRequester.Defaultas the restorer fallback: it arms at resume, one composition after the shell's synchronous claim, so naming its requester handed the restorer a detached node — the throw was swallowed and the claim degraded silently. Card disposal now clears the attachment latch rather than setting a terminal one, becauseLazyRowdisposes on ordinary viewport recycling and was abandoning restores that would have succeeded. The per-root restore flags collapse into onedetailReturnRoot.Budgets, previously 2.4 s in one place and ~100 ms in two others. Now named and justified: acquisition (nothing focused yet) stays at 2.4 s because exhaustion means a dead D-pad; relocation (focus already usable, moving it somewhere better) is 480 ms because exhaustion degrades to a working fallback and a longer budget only buys focus thrash. For You and Diagnostics move onto the relocation budget.
Minor: selector collapse is derived at read time instead of deferred an effect behind;
nextTvDiagnosticsCrashFocusreturns null for a control outside the current order instead of coercing the miss to index 0 and sending Down upwards; top-anchor position events are conflated; the constanttargetStateargument is defaulted away; For You'sonItemClickbecomesonSavedListItemClick; source-guard tests match whitespace-insensitively so they assert the rule rather than a formatting snapshot.Verification
./gradlew :androidTvApp:testDebugUnitTest :shared:testDebugUnitTest— 819 + 978 tests, 0 failures./gradlew :androidTvApp:assembleDebug :androidApp:assembleDebugStacking
This PR is stacked on #164. Merge #164 first; GitHub will then reduce this diff to the focus-foundation commits. PR #164's fork branch has also been fast-forwarded with the reviewed remediation commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests