fix(tv): Back out of a cascade lands on its own tab, without flashing through search - #215
Merged
RXWatcher merged 10 commits intoAug 11, 2026
Merged
Conversation
Back from content on a tab root asks the bar to take focus via a token bump, and nothing checks whether it landed. When it does not, every subsequent Back re-evaluates to the same request: MenuBack is never reached and Home and exit become unreachable. Reproduced on a Google TV Streamer — four consecutive 'focus request -> menu' one second apart with no 'focused -> menu' between them. The state now records that the handoff was asked for, and a second Back escalates to MenuBack rather than repeating a request that is evidently not working. The flag clears when the bar reports focus or when a panel close hands focus somewhere deliberate, so the normal climb-to-bar step is unchanged. Silo-Server#204 gave the content direction an observed handoff with a fallback; this is the same failure in the bar direction, which it did not cover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Back on Search is consumed to send focus to the search field. If the field never reports focus, every Back repeats that and Search cannot be left — the same unobserved-claim loop as the bar handoff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MenuBack on Home means 'exit', so escalating there turned a Back the viewer expected to move focus into quitting Silo. Observed on the Streamer: Back from a cascade dropped straight out to the Google TV home screen. Escalate only where MenuBack navigates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bar skipped the whole focus request when the explicit target was unavailable, so nothing was focused and Compose's default search landed on the first bar element — the search icon. Back out of a cascade therefore appeared to 'go to search', and pressing centre opened it. Availability now gates only the explicit target; the request still runs and falls back to the selected entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Telemetry from the Streamer shows the loop: close -> root_panel, focused -> menu, then preview -> root_panel ~250ms later, and the next Back spent closing that preview. MenuBack is never reached, so Home is unreachable. Crucially there is no 'request -> menu' between the close and the focus — Compose restores focus to the anchor itself when the panel leaves composition, so the explicit request never ran and suppressDwellPreview was never armed. Silo-Server#204 removed the path that used to arm it, treating it as dead. ClosePanel now requests the anchor with suppression, and the shell stops claiming content on that branch: it was fighting a restore it could not win, and losing it silently was what dropped the suppression. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Back took the ClosePanelPreview branch (panelHasFocus false even after enter), which deliberately moves no focus. With no explicit request the bar falls back to selectedEntryRequester() — the selected tab, or the search icon when the route is Search — so backing out of Movies' cascade landed on Home. Request the anchor with dwell suppressed on that branch too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closing a cascade removes the focused node, and Compose recovers focus a frame before any request of ours can land — it picks the bar's first child, the search icon. Nothing written to state can win that frame: arming suppression, restricting focusability and pointing the group's enter at the anchor were all applied a recomposition too late, and one attempt at making the request immediate regressed where focus rests. So move focus while the panel is still composed, then close it. There is no recovery left to lose. The deferred request stays as the fallback for when the bar has not installed its hook. Verified on a Google TV Streamer: Movies cascade -> Back now rests on the Movies tab with no intermediate claim, where before it flashed through search and, without the suppression, re-previewed the cascade it had just dismissed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 40 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 TV shell now limits failed Search refocus attempts, restores focus to panel anchors during Back handling, and tracks unanswered menu-bar handoffs. The top menu gates focusable controls during recovery and applies bounded dwell suppression. Unit tests cover successful and failed recovery paths. ChangesTV Back and focus recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TvMainShell
participant TvShellFocusState
participant TvTopMenuBar
User->>TvMainShell: Press Back
TvMainShell->>TvShellFocusState: Route Back action
TvShellFocusState->>TvTopMenuBar: Request anchor focus
TvTopMenuBar-->>TvShellFocusState: Confirm or reject focus claim
TvShellFocusState->>TvMainShell: Close panel or continue navigation
Possibly related PRs
Suggested reviewers: 🚥 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 |
requestFocus() returns whether the claim was accepted, so
runCatching{}.isSuccess threw the real answer away — it is true for any
call that merely did not throw. A refused claim therefore looked like a
move: suppression was armed, the panel closed, and the deferred fallback
was skipped, leaving focus nowhere until the next Back. Route it through
claimFocusOrReport, which returns acceptance and reports a refusal
instead of swallowing it.
Found by Codex review of Silo-Server#215.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
claimFocusOrReport returns acceptance; its own docs are explicit that accepted is not arrival. Calling the result 'moved' claimed more than the helper can know. The behaviour is unchanged — acceptance is still the right gate, because it separates a claim that took from one that definitely needs the deferred retry. 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: 1
🧹 Nitpick comments (1)
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvShellFocusState.kt (1)
252-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the focus-recovery documentation with the new behavior.
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvShellFocusState.kt#L252-L262: State thatfocusBarAnchorNowreturns focus-claim acceptance, not observed focus arrival.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvShellFocusState.kt#L406-L410: State that Back restores focus to the panel anchor throughclosePanelOntoAnchor(), not to content.🤖 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/shell/TvShellFocusState.kt` around lines 252 - 262, Update the documentation for focusBarAnchorNow to clarify that its Boolean result indicates focus-claim acceptance, not confirmed focus arrival. Also revise the Back recovery documentation at androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvShellFocusState.kt lines 406-410 to state that closePanelOntoAnchor() restores focus to the panel anchor rather than content.
🤖 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/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/shell/TvShellFocusStateTest.kt`:
- Around line 319-340: Add a focused test in TvShellFocusStateTest covering
onBack with barHandoffAttempted already true and onHome true; assert that it
returns TvShellBackAction.MoveFocusToMenu, preserving the Home handoff exception
instead of escalating to app exit.
---
Nitpick comments:
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvShellFocusState.kt`:
- Around line 252-262: Update the documentation for focusBarAnchorNow to clarify
that its Boolean result indicates focus-claim acceptance, not confirmed focus
arrival. Also revise the Back recovery documentation at
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvShellFocusState.kt
lines 406-410 to state that closePanelOntoAnchor() restores focus to the panel
anchor rather than content.
🪄 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: 6a636d21-c9c7-474e-adaf-1f1412d282b5
📒 Files selected for processing (4)
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvMainShell.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvShellFocusState.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvTopMenuBar.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/shell/TvShellFocusStateTest.kt
MenuBack on Home means EXIT, so escalating an unanswered handoff there turns the second Back into a silent app exit — which an earlier cut of this branch did, and which is worse than the stranding the escalation fixes. The guard existed; nothing held it in place. Raised by CodeRabbit on Silo-Server#215. 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.
Back out of a top-bar cascade did not leave the viewer where they expected, and Home could be unreachable. Found and fixed against a Google TV Streamer, using the focus telemetry to see what actually happened rather than what the code implied.
What was wrong
Back landed on the wrong tab. Backing out of an entered cascade took a path that moved no focus at all, so the bar fell back to
selectedEntryRequester()— the selected tab. Back out of Movies' cascade and you landed on Home. On the Search route the same fallback is the search icon, which is where the "Back goes to search" reports came from.The cascade sprang straight back open. Once the anchor was named, its dwell preview re-opened ~250ms later, so the next Back was spent closing that preview instead of reaching
MenuBack— and Home stayed unreachable. The suppression that should have prevented this was armed, then disarmed by a transient focus on the bar's first child while the request was still being applied.Back visibly flashed through the search icon. Closing a cascade removes the focused node, and Compose recovers focus a frame before any request of ours can land — it picks the bar's first child. Nothing written to state can win that frame. Arming the suppression, restricting focusability during the handoff, and pointing the focus group's
enterat the anchor were each applied a recomposition too late; making the retry helper's first attempt immediate did remove the flash but regressed where focus comes to rest, so it was reverted.The fix
Move focus to the anchor tab while the panel is still composed, then close it. There is no recovery left to lose, so there is no intermediate claim to flash through. The existing deferred request stays as the fallback for when the bar has not installed its hook or the anchor is not focusable yet, and a short grace period keeps a non-anchor focus from disarming the suppression during the handoff.
Verification
:androidTvApp:testDebugUnitTestpasses, including new cases covering the synchronous hook, the fallback when it cannot move focus, and the anchor/suppression contract. Two existing tests asserted the old "do not move focus" contract and were updated deliberately.close -> root_panel,dwelleff -> f_null_s_Tab(Movies),focused -> menu,dwelleff -> f_Tab(Movies)_s_Tab(Movies). No search in the trace, no re-preview, and Back rests on the Movies tab.🤖 Generated with Claude Code
https://claude.ai/code/session_01Q9UyH5fvug685dzUFLtdpW
Summary by CodeRabbit
Bug Fixes
Tests