fix(tv): make the crash-report prompt reachable on a television - #202
Merged
RXWatcher merged 3 commits intoAug 10, 2026
Merged
Conversation
The crash-consent dialog rendered with every button focusable and none focused; the only focused node was a card in the Home row behind it. A leanback app takes no touch input, so there was no D-pad path and no tap fallback — the dialog was unreachable, and with it the only route to sending a report. GlitchTip held zero crash events despite a confirmed FATAL EXCEPTION on the device: crash reports have never been sendable from an Android TV. Cause is the pattern Silo-Server#199 addressed elsewhere — a single first-frame `runCatching { requestFocus() }`. requestFocus() throws rather than returning false when its node has not attached, and this prompt composes immediately after a crash, when the tree is the least settled it will ever be. Retries after a frame, for both the prompt and its confirm step. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit a0d24cc6e983a8bac4629a04f97d8fa52bc2d86c)
…ch it The prompt was composed as a sibling after the NavHost, which put it inside the shell's content Box — and that Box carries a focusRestorer. A restorer intercepts focus *entry* into its subtree and redirects it to the child it remembers, so every claim the prompt made was rerouted to whatever card the viewer had last used. That is why a card behind the dialog held focus while every button in front of it rendered focusable and unfocused. Neither retrying the claim nor adding a focus boundary inside the subtree could win: both govern movement once focus is in, and it never got in. A leanback app takes no touch, so there was no fallback either — the dialog was unreachable, and with it the only route to sending a crash report. GlitchTip held zero crash events tonight despite a confirmed FATAL EXCEPTION on the device. A Dialog gets its own window and its own input focus, which is what a modal asking a yes/no question needs. mCurrentFocus now resolves to the dialog window and the options are selectable. Note for anyone verifying this: uiautomator's focused="true" is not a reliable signal for Compose focus inside a dialog window — it kept reporting only containers while the D-pad worked fine. Check mCurrentFocus in dumpsys window instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit b4703a9d13fe2ea954d2252300306d1629252b78)
|
Warning Review limit reached
Next review available in: 12 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 (1)
📝 WalkthroughWalkthroughThe diagnostics prompt now renders in a platform ChangesDiagnostics modal focus
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/screens/settings/diagnostics/TvDiagnosticsPromptScreen.kt`:
- Around line 91-94: Update the DialogProperties configuration in
TvDiagnosticsPromptScreen to set dismissOnBackPress to false, preventing the
non-dismissable diagnostics prompt from invoking onDismissRequest and onDontSend
through platform Back navigation.
🪄 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: 47d8ab2d-6606-4735-8a1a-4001517b8069
📒 Files selected for processing (1)
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/diagnostics/TvDiagnosticsPromptScreen.kt
RXWatcher
added a commit
to RXWatcher/silo-android
that referenced
this pull request
Aug 10, 2026
The first draft claimed the screen-local half of the 2026-08-04 design "works" and only the shell needed modelling. Measuring instead of asserting says otherwise, and it reverses the ordering. Focus churn by area over 90 days: ui/screens/ 150 file-touches, ui/components/ 71, ui/focus/ 42, ui/shell/ 31. TvMainShell is the most-edited single file, but screens are five times the churn. The cause is not a missing model. It is a model nobody adopted: 8 files use the shared bounded observed-focus policy, 44 still call requestFocus() directly, and 107 runCatching occurrences remain in TV screens. Eighteen percent adoption. Cause #1 of that audit — "a focus request executing without exception is treated as focus acquisition" — is not a finding being worked off, it is the majority of the current code, and it is the same failure mode as Silo-Server#199 and Silo-Server#202. So: enforcement first (a source test that fails on runCatching around requestFocus in TV screens, baselined and ratcheting down), migration second in churn order (detail 30, player 29, audiobook 21, calendar 14, library 12 — 106 of the 150), shell ownership model third. A better rule that 18% of the code follows is worth less than the existing rule made impossible to violate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The prompt itself was migrated to requestFocusUntilObserved, but its confirm step kept a fixed try / wait-a-frame / try-again. That is still blind: it cannot distinguish a claim that landed from one that was dropped, which is the whole failure this PR exists to remove. It is also the second step of the only flow that lets a viewer send a crash report, so a dropped claim there strands them on a dialog with nothing focused and no touch fallback. Now the same observed policy as the prompt, with the container reporting focus through onFocusChanged. Found by checking this branch against the ratchet added in the focus-gate PR — the standard flagged its own author's work, which is the point of having it. Verified: :androidTvApp:testDebugUnitTest 975 tests 0 failures, APKs assemble. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RXWatcher
added a commit
to RXWatcher/silo-android
that referenced
this pull request
Aug 10, 2026
Silo-Server#202 removed the two TvDiagnosticsPromptScreen claims this baseline was holding open, so the ratchet reports 0 and the constant follows it. Every TV screen is now free of silent focus claims, and any new one fails the build. Verified: :androidTvApp:testDebugUnitTest 976 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RXWatcher
added a commit
that referenced
this pull request
Aug 10, 2026
) * test(tv): ratchet against silently-failing focus claims in screens `requestFocus()` throws when its node has not attached yet rather than returning false, so `runCatching { requestFocus() }` does not handle that failure — it hides it. Focus goes nowhere, no exception surfaces, nothing is logged, and on a leanback app there is no touch fallback to recover with. That is cause #1 of the 2026-08-04 whole-application focus hardening design, and it is still the majority of the code four months later: 8 files use the shared bounded observed-focus policy, 44 still call requestFocus() directly. Eighteen percent adoption. It is also the exact mechanism behind #199 (content focus entry found nothing to focus) and #202 (the crash-report prompt was unreachable, so crash reports have never been sendable from a television). Focus fixes are accelerating rather than converging — 8 focus commits on main in June, 26 in July, 49 in the first ten days of August, 49 of the last 90 days' 83 being `fix:`. A better rule that 18% of the code follows is worth less than the existing rule made impossible to violate, so this makes the next instance fail the build. It does not fix the 78 existing sites. It stops the 79th, while they are migrated in churn order (player 10, detail 8, settings 7, recommendations 7). Equality rather than `<=` on purpose: a `<=` ratchet leaves slack that the next silent claim quietly fills. Migrating a site means lowering BASELINE in the same commit, and the failure message says so. Verified both directions: passes at 78, and fails with the explanatory message when a claim is added — a ratchet that cannot fail is decoration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(tv): point the gate's message at the file that declares the helper requestFocusUntilObserved is declared in ui/focus/TvObservedFocusPolicy.kt, not TvContentInitialFocus.kt (which calls it). A developer following the failure message would have opened the wrong file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): observe focus acquisition in the intro banner and the HUD picker Two of the player's ten silent focus claims, migrated to rememberTvContentInitialFocus. Baseline 78 -> 76. Both are the pattern the ratchet exists for: a LaunchedEffect on first composition wrapping requestFocus() in runCatching, which does not handle the throw — it hides it. The intro banner is the sharper case, because it composes into a fresh AnimatedContent subtree on every state transition, so it makes its claim at precisely the moment the tree is least settled. When that claim is dropped the countdown shows with nothing focused and a Select press does not cancel the skip. Stopping at two rather than doing all ten, deliberately. The remaining eight are D-pad-critical control flow — playPauseFocus, scrubberFocus, rootFocus and primaryFocus in TvPlayerScreen, and the HUD tab-pill seeding whose target is a map lookup that varies with the selected tab. Those want an on-device pass before they move, because the failure mode of getting one wrong is a player whose transport controls cannot be reached, which is worse than the silent claim being fixed. These two are self-contained popups whose container is unambiguous. Verified: :androidTvApp:testDebugUnitTest 976 tests, 0 failures, and the ratchet reported the new count itself rather than being told it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): observe focus in the card-overlay preview and the inbox Three more sites, 76 -> 73, all in static screens where the target container is unambiguous. Card overlay settings: when the overlay feature is switched off the detail panel stops being focusable and the D-pad goes dead, so focus moves to the preview pane. That is a relocation — focus already exists and is about to be invalidated — so it takes the short frame budget rather than the acquisition one; a long budget there would only mean seconds of visible thrash. Inbox: two claims with different characters. The first is acquisition, when the list has just populated and nothing is focused yet — dropping it leaves a dead D-pad on a full screen of notifications. The second is relocation after Mark-all removes the focused card from composition. Success is observed as "focus is inside the inbox" rather than "the first row specifically", because a claim landing anywhere in the list is what keeps the D-pad working, and that is what the retry is protecting. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble, and the ratchet reported 73 itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): observe focus in person detail's filter chips and bio modal Three more sites, 73 -> 70. The filter row has both flavours in one file. Entry is acquisition — the page has just loaded and nothing is focused. The refocus after a filter change is a relocation, and a sharp one: key() disposes the whole grid including the chip the viewer just pressed, so the claim lands on a node that is being recreated underneath it. Short budget for that one, generous for entry. The full-bio modal is plain acquisition behind a 50ms delay that was doing the retrying by guesswork. The filter observation is taken on the header Column rather than the chip itself: the chip lives in a separate composable that receives only the requester, and "focus is in the header" is the criterion these retries are actually protecting. Threading a callback through just to observe one node would be more API for no more truth. Also records a limitation found while doing this. Person detail's popup-dismiss restore lives in DisposableEffect { onDispose { … } }, which is not a suspend context, so no retry loop can run there — the policy cannot be adopted at that site at all. Sites of that shape need a different answer, and the ratchet counting them means its floor is not zero. Better stated in the baseline than discovered by whoever tries to finish the migration. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble, ratchet reported 70 itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): observe focus on the first-run, signup and login surfaces Four more sites, 70 -> 66. All acquisition: a form has just appeared and nothing on it holds focus, so a dropped claim leaves a remote with nothing to act on and no touch fallback to recover with. First-run setup is the worst of them — a viewer whose very first screen ignores the D-pad has no reason to assume the app works at all. Setup and signup take rememberTvContentInitialFocus, since each has a single target under one root. Login needs the policy directly: its target depends on which surface is showing, and the claim re-fires when they swap. The phone-first branch is the one that matters — a dropped claim there strands the remote on a QR code that cannot be actioned. Both targets live under the same root, so observation is taken there. The target is hoisted to a val rather than selected inline at the call, because `usernameFocus::requestFocus` inside an if/else resolved to the FocusDirection overload rather than the no-arg one. Worth knowing before the same shape appears in the remaining sites: a bound reference to requestFocus is ambiguous without an expected type on hand. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble, ratchet reported 66 itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): stop the library grids reporting a focus handover that did not happen Three sites, 66 -> 63 — and two of them were doing something worse than claiming focus silently. Both library grids called onInitialContentFocus() unconditionally, right after an unobserved requestFocus(). That callback is how a screen tells the shell it has taken content focus. Firing it when the claim was dropped tells the shell focus landed somewhere it did not, and the policy's own documentation names the consequence: "telling a shell that focus landed when it did not is how a screen ends up with no focus owner at all". Nothing focused, and the shell believing otherwise, so nothing corrects it. The handover now fires only on observed acquisition. The claim itself is observed on the grid, since "focus is in the grid" is what the retry is protecting rather than the first cell specifically. Collection detail is the plain case: initial focus on a list that has just populated, previously blind behind its own guard flag. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble, ratchet reported 63 itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): observe calendar's shelf handover and retire its hand-rolled retry Three sites, 63 -> 60. The shelf request is the library-grid bug again: onFocusApplied() retires the pending focus request, and it was called straight after an unobserved claim. Retiring a request whose claim was dropped loses it entirely — nothing focused and nothing left to retry it. It now fires only on observed acquisition, using the shelf's existing focus reporting, which was already sitting three lines below the effect. The day claim was a hand-rolled six-attempt loop pacing itself with frames and 40ms delays, judging success on requestFocus() returning true. That is acceptance, not arrival: it reports that the request was taken, not that focus is there. The shared policy does the same pacing and judges it on observed focus, so the bespoke loop goes. Two calendar sites are deliberately left. The NavHost-restore handoff at the top of the screen coordinates with shell bar suppression across several frames and wants its own change. The Up-fallback claim lives in a key-event branch that must return synchronously whether it handled the key, so it has no suspend context to retry in — the same shape as person detail's onDispose restore. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble, ratchet reported 60 itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): migrate settings, and remove the "cannot be migrated" category Six sites, 60 -> 54, including the first one that had no coroutine to retry in. Settings' entry focus was four attempts judged on requestFocus() returning true, then onInitialContentFocus() called regardless — so the shell was told content had taken focus even when the loop had just failed four times running. Observed now, and the handover fires only on arrival. The rail already reported a category taking focus, so that signal is routed to the screen rather than adding a layout node to watch for it. Also migrated: the detail-pane request, the picker dialog's initial focus, and the destructive-confirm dialog, where Cancel holding focus is what stops a stray Select press running the destructive action. The Back-to-category claim is the interesting one. A BackHandler must return synchronously whether it consumed the key, so it cannot await anything — which is what I had been treating as an exemption, twice. That was wrong. Retrying is only half of what the policy provides; the other half is that a failed claim stops being invisible, and that half needs no coroutine at all. So there is now claimFocusOrReport for those callers: one attempt, because one attempt is all they can make, and a diagnostic when it does not land instead of a swallowed throw. The same tool covers person detail's onDispose restore and calendar's Up-fallback branch, which were the other two "unmigratable" sites. The baseline's floor is zero after all. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): migrate the synchronous claims and library browse controls Five sites, 54 -> 49. Person detail's onDispose restore and calendar's Up-fallback key branch are the two sites I had twice called unmigratable. Both now use claimFocusOrReport: one attempt, since that is all a teardown or a key handler can make, and a reported failure rather than a swallowed throw. Library's clear-filters pill is the same shape — clearing filters removes that pill from composition, so focus is moved off it inside a click handler with no suspend point. The sort and facet panels are ordinary acquisition behind a 50ms delay that was doing the retrying by guesswork. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): observe every focus claim on the search screen Six sites, 49 -> 43, and search was the densest file yet — a text field, filter chips, catalog results, request rows and a feedback action all competing for one screen's focus. The four-way post-search claim was a single runCatching wrapping an if/else chain, so whichever branch it picked, a throw from any of them was swallowed identically. The target is now chosen first and claimed once, which also makes the choice readable. Both return restorations previously waited exactly one frame and hoped. They are relocations onto cards that are being scrolled into place underneath them, so they take the short budget and are judged on arrival. Back from a raised keyboard uses the single-shot claim: it has to answer synchronously whether it consumed the press, and losing that claim quietly would leave the viewer with the keyboard gone and nothing focused — which is the failure that made Back pop the whole screen before. Observation is taken at the screen root, since every one of those targets lives under it and 'focus is on search' is what each claim is actually waiting for. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): migrate the recommendations focus bridge and For You entry Seven sites, 43 -> 36. Six of them are Boolean-returning lambdas handed to the focus bridge and to a row's DirectionUp handler. Each wrapped requestFocus in runCatching and defaulted to false, so a throw and a genuine refusal were indistinguishable to the bridge deciding what to do next. They now report the difference while still answering synchronously, which is all a bridge callback or a key handler can do. The seventh is the For You entry claim, and it is the fifth false shell handover this sweep has turned up: onInitialContentFocus() fired whether or not the claim landed. Same fix as the library grids, the calendar shelf and settings — the handover waits for observed arrival. Five occurrences of one bug across five unrelated screens is not five mistakes. It is what happens when the only available primitive cannot report failure, so every caller assumes success. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): migrate admin, browse, home and the audiobook bookmark panel Five sites, 36 -> 31. Home is the sixth false shell handover: onInitialContentFocus() fired straight after an unobserved claim, so a dropped claim on the app's first screen left nothing focused and the shell believing content owned focus. Admin hub, admin user edit and browse are ordinary acquisition. The audiobook bookmark delete moves focus to a stable anchor because the deleted row leaves composition — a click handler, so single-shot and reported. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): migrate the profile form key handlers and the requests screen Six sites, 31 -> 25. The profile form's three DirectionDown handlers each claimed focus and returned true unconditionally — reporting the key as consumed whether or not focus had moved, so a refused claim ate the press and left the viewer stuck on the field above. Single-shot and reported now, and the handler's answer follows the claim. Requests' entry claim is the seventh false shell handover. Its post-search target was an if/else around two separate runCatching blocks; the target is chosen first and claimed once. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): migrate item detail, including the isSuccess focus check Eight sites, 25 -> 17. The return-to-top handler asked runCatching{ requestFocus() }.isSuccess. That is true whenever the call did not THROW, so a request that returned false — node present but refusing focus — counted as focused. The scroll then ran as though the highlight had already moved, which is the 'focus appears only after the window settles' symptom the surrounding comment was written to prevent. Both claims now report the request's own answer. The cast return-restore was a hand-rolled forty-attempt loop, and unusually it was already judging on observed focus — which is why it worked. It just open-coded the pacing, so the policy replaces it with the two-frame cadence and attempt count preserved. The two similar-restore lambdas are owned by TvSimilarFocusRestoration, which does its own observation and documents that the return value is not evidence. They keep answering synchronously and now report a swallowed throw. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): migrate the player HUD and transport focus claims Eight sites, 17 -> 9. This is the set I had been deferring for a device pass. The sharpest is the hidden-overlay root claim: while controls are hidden the outer Box must own focus or the first remote press never reaches onPreviewKeyEvent — the viewer presses once, nothing happens, and presses again. That claim was unobserved, so on any frame where the Box had not attached it silently did not happen. The idle overlay's target was chosen inside a single runCatching wrapping a when, so a throw from either branch was indistinguishable. Chosen first, claimed once. Both transport handoffs (scrubber down to play/pause, transport up to scrubber) and the remote's FocusTransport action answer synchronously, so they take the single-shot reporting claim. Still not device-verified — the D-pad paths through the transport want a real remote, and that is a check I cannot run from here. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): eliminate the last silent focus claims — baseline reaches zero Nine sites, 9 -> 0. Diagnostics settings ran a bounded loop through a helper that mapped a Result: not throwing counted as FOCUSED, so it stopped on acceptance rather than arrival. Its key handler moved between rows and reported the press consumed regardless. The crash prompt and its confirmation now retry against observed focus, matching the treatment on the crash-prompt-reachability branch so the two resolve to the same shape whichever lands first. Server setup's host field and, more importantly, the pairing consent prompt — an Allow button that never takes focus cannot be answered from a remote at all. Calendar's NavHost-restore handoff was the one I deferred twice. It coordinates bar suppression across several frames, and its arrival signal turned out to be already present: the controls report which zone took focus, so no new node was needed to watch for it. The ratchet now guards zero. Any new runCatching { requestFocus() } in a TV screen fails the build, and between them the two tools cover every context: requestFocusUntilObserved where a coroutine exists, claimFocusOrReport where the caller must answer synchronously. Not device-verified. The unit suites prove nothing regressed and the APKs build; whether each claim now lands on the right node under a real remote is a separate pass. Verified: androidTvApp 976 tests, android-shared 1101 tests, 0 failures, 4 APKs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(tv): leave the crash prompt to the branch that can actually fix it Backs out this branch's changes to TvDiagnosticsPromptScreen and raises the baseline from 0 to 2. Migrating those two claims here was duplicated work, and worse than duplicated: observing the retry is the half that does not fix them. The prompt is composed inside the shell's content Box, whose focusRestorer intercepts focus ENTRY and reroutes it to the child it remembers, so a retry loops into the same interception however many times it runs. What fixes it is giving the prompt its own Dialog window, which lives on the crash-prompt-reachability branch along with tvModalFocusBoundary and the modal restore attempts. Had both landed, the resolution would have had to pick one, and picking this branch's version would have left the code looking correct while the prompt stayed unreachable — the exact failure mode that made crash reports unsendable from a television in the first place. So that file now belongs to one branch. Drop the baseline to 0 when it lands. Verified: :androidTvApp:testDebugUnitTest 976 tests 0 failures, APKs assemble. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test): correct the stale site count and record the ratchet's two limits The KDoc still claimed 78 sites remained and that the test stops the 79th; the baseline has moved with the migration and now records what is left rather than what it started at. Also records two real limits raised in review, both of which hold only because the baseline is at or near zero. The scan is a fixed character window rather than a brace-aware parse, so it can pair a runCatching with an unrelated requestFocus — which happened during this migration — and can miss one written further away than the window. And the assertion compares a total rather than a set, so while the baseline was non-zero, adding one claim while migrating another kept the count and passed. At zero there is nothing to offset against, which is the only reason a count suffices; if the baseline is ever raised above zero the hole reopens. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(tv): baseline reaches zero now that the crash prompt has landed #202 removed the two TvDiagnosticsPromptScreen claims this baseline was holding open, so the ratchet reports 0 and the constant follows it. Every TV screen is now free of silent focus claims, and any new one fails the build. Verified: :androidTvApp:testDebugUnitTest 976 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: rxwatcher <rxwatcher@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Silo's own crash-consent prompt (
TvDiagnosticsPromptScreen, Review / Send / Always send →DiagnosticsUploadWorker) renders on TV with every button focusable and none focused — the only focused node is a card in the row behind it.It is composed as a sibling after the
NavHost, which puts it inside the shell's contentBox, and thatBoxcarries afocusRestorer. A restorer intercepts focus entry into its subtree and redirects it to the child it remembers, so every claim the prompt makes is rerouted to whatever card the viewer last used. A leanback app takes no touch input, so there is no fallback either.The consequence: crash reports have never been sendable from an Android TV. A confirmed FATAL EXCEPTION on a real device produced zero reports.
Neither retrying the claim nor adding a focus boundary inside the subtree can win — both govern movement once focus is in, and it never gets in.
The fix
Two commits:
runCatching { requestFocus() }throws rather than returning false when its node has not attached — and this prompt composes immediately after a crash, when the tree is the least settled it will ever be.Dialog. A dialog gets its own window and its own input focus, which is what a modal asking a yes/no question needs;mCurrentFocusthen resolves to the dialog window and the options are selectable.Verification
:androidTvApp:testDebugUnitTest— 975 tests, 0 failures. All 4 debug APKs assemble.Note for anyone verifying on device: uiautomator's
focused="true"is not a reliable signal for Compose focus inside a dialog window — it reports only containers while the D-pad works fine. CheckmCurrentFocusindumpsys windowinstead.One file changed,
TvDiagnosticsPromptScreen.kt.Summary by CodeRabbit