fix(tv): observe focus arrival per target, not per screen - #216
Conversation
requestFocusUntilObserved tests isFocused() BEFORE it ever calls requestFocus, so an arrival test broader than the thing being asked for turns the whole claim into a no-op. Five sites got this wrong, in two directions. Always-true — the claim never fires. Search and Requests both waited on a screen-root hasFocus, which is already true the moment you are on the screen at all: - Back from a result never returned to the search field, because a result having focus is exactly the state that satisfied the test; - a submitted search never handed you its results, because the field had focus; - return-restoration onto the card you left from was a coin flip on whether the root had reacquired focus yet. Always-false — the claim can never be confirmed. Calendar's calendarFilterHasFocus was declared and never assigned, so it read false forever: the claim burned every attempt and reported Exhausted even when focus had landed, and the reconfirm after Android's delayed focus pass, the bar-suppression release and onInitialContentFocus() never ran. Each claim is now observed on the region it actually asked for, so moving focus WITHIN a screen is a state the test can distinguish. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 19 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 (3)
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 |
… item identity Two holes in the per-region observation. The calendar's zone callbacks only fire when a control or shelf GAINS focus, so moving Up from the filter into the top menu left the flag true with focus outside the screen entirely. A later shell handoff read that stale true, skipped both claims, and still reported initial content focus — and only another calendar zone gaining focus could clear it, which the skipped handoff could never cause. Clear it when the screen loses focus. Search's result-region callbacks are per card, and Compose can deliver the newly focused card before the outgoing one reports false, so the card that just lost focus cleared the region the new one had set; a recycled item has the same shape. Guard the clear on item identity, the way the neighbouring return-target tracking already does, so a stale false whose id is no longer current is ignored. Found by Codex review of Silo-Server#216. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A return is a claim on one specific item, so testing 'some result has focus' is the same too-coarse arrival this change removed everywhere else: any already-focused card in the region satisfied it, and the saved card was never requested — precisely the case a return exists to serve. The identity wait below already knew the right answer; the claim above it did not. Found by Codex reviewing this branch against the new main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
requestFocusUntilObservedtestsisFocused()before it ever callsrequestFocus:repeat(maxAttempts) { awaitAttempt() if (isFocused()) return TvObservedFocusResult.Focused // <- no request is made ... }So an arrival test broader than the thing being asked for turns the whole claim into a no-op. Five sites got this wrong, in two opposite directions. This is the same defect fixed for the player overlay in #214; these are the rest of them.
Always true — the claim never fires
TvSearchScreenandTvRequestsScreenboth waited on a screen-roothasFocus, which is already true the moment you are on the screen at all. The comment at the oldTvSearchScreenobserver said so outright — "every focus target on this screen lives under here, so 'focus is on the search screen' is the arrival each claim below waits on" — which is the wrong granularity as soon as a claim has to move focus within the screen.Always false — the claim can never be confirmed
TvCalendarScreen.calendarFilterHasFocuswas declared and never assigned anywhere, so it readfalseforever. The claim burned every attempt and returnedExhaustedeven when focus had landed, soappliedwas always false — and the reconfirm after Android's delayed focus pass, the bar-suppression release, andonInitialContentFocus()never ran.Fixing it needed the control-focus callback widened to carry which zone took focus (and
nullwhen the controls lose it): "some control got focus" cannot distinguish the filter row from the week strip, and the filter claim is observed on the filter row specifically.The shape of the fix
Each claim is observed on the region it actually asked for, so moving focus within a screen is a state the test can distinguish. Search and Requests get a small
FocusRegionenum fed by the existing per-item and field focus callbacks, plus one observer each on the chip rail and (for Search) the feedback action.Verification
:androidTvApp:testDebugUnitTestpasses.input textnor hardware keyevents reach it), so the Search and Requests paths were verified by reading, not by pressing. Calendar likewise.🤖 Generated with Claude Code
https://claude.ai/code/session_01Q9UyH5fvug685dzUFLtdpW