docs: design an explicit focus-ownership model for the TV shell - #207
docs: design an explicit focus-ownership model for the TV shell#207RXWatcher wants to merge 3 commits into
Conversation
…n load
An episode payload carries no favourite flag, so the detail screen asks about
each episode individually — `GET /favorites/{id}`, which answers 404 for "not
a favourite". Two things made that expensive enough to see from the field.
Every episode was asked on every season load, including episodes whose answer
was already on screen: the previous state map was consulted only as an
error fallback and then replaced wholesale, so switching to season 2 and back
re-asked all of season 1. And all of them went out at once, unbounded.
On a tester's Fire TV one series produced 116 of these 404s — 150-520 ms each,
and the largest single source of 4xx the client emitted in the session.
Now only episodes with no answer yet are asked, six at a time. The map
accumulates for the life of this view model, which is one visit to one item,
so leaving the screen and returning still re-reads: a favourite toggled on
another device is picked up on the next visit rather than cached indefinitely.
A failed probe is left unrecorded rather than stored as false, so a transient
error cannot stick as a cached "not a favourite" for the rest of the visit —
the old code wrote the fallback into the map, which could.
This is the client half. The server half is that episode `user_data` could
carry `is_favorite` the way item-level `ItemUserState` already does, which
would remove the probes entirely; that is a separate change in silo-server and
this one stands on its own without it.
Verified: :androidTvApp:testDebugUnitTest 975 tests, 0 failures.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Focus fixes are accelerating, not converging: 8 focus commits on main in June, 26 in July, 49 in the first ten days of August. Of the last 90 days' 83, forty -nine are `fix:` against nine `feat:`. TvMainShell.kt has been edited by a focus commit fifteen times. The 2026-08-04 whole-application hardening design was largely right and this keeps its principles. But it explicitly left the shell unmodelled — "screen- specific resolution remains close to the screen" — and the shell is where per- screen policies collide. It also has no enforcement half, and an escape clause that makes divergence compliant. Two defects found since are not among its six causes, and neither is an acquisition bug: a focusRestorer on an ancestor intercepting focus ENTRY so no subtree fix can win (Silo-Server#202), and one field meaning both "which element" and "suppress its dwell" (Silo-Server#204). Both are ownership bugs. TvShellFocusState carries ten mutable fields and none of them says who owns focus; ownership is inferred by reading several together, every combination is representable including the meaningless ones, and each fix adds a flag excluding one bad combination — enlarging the space the next defect hides in. The proposal is one value: an owner (Content/Bar/Panel/ProfileMenu/Modal) and why focus arrived there, with Back and dwell as total functions over it. That turns b44e1d8f's own stated insight — "the distinction that matters is not which call site but why focus arrived" — into something the type system holds, so adding an owner breaks compilation at every decision point. That compiler enforcement is what the previous design lacked. Scoped to the shell. Screen-local acquisition and the bounded observed-focus policy stay as they are; that half works. Success criteria are stated and measurable, including the GlitchTip signals real testers already produce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 27 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 (2)
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 |
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>
|
Superseded by #211. This was stacked on the pre-merge version of #206 and carried its original commit #211 is the same design note rebuilt on merged |
1 participant
Design only — no code changes. Opening it as a PR so the argument can be
disagreed with before anyone refactors the shell.
The problem
Focus fixes are accelerating, not converging.
mainOf 83 in the last 90 days, 49 are
fix:against 9feat:.TvMainShell.kthas been edited by a focus commit 15 times. There are already 19 production focus files, 22 focus test files and 8 focus design docs — the infrastructure is not missing.Why the last design did not stop it
2026-08-04-whole-application-focus-hardening-design.mdwas largely right and this keeps its principles. But it says:Reasonable — but it leaves the shell unmodelled, and the shell is where per-screen policies collide. It also has no enforcement half, and its escape clause ("existing specialized flows … remain intact unless they can adopt the helper") makes divergence compliant.
Two defects found since are not among its six causes, and neither is an acquisition bug:
focusRestoreron an ancestor intercepts focus entry, so no subtree-level fix can win.menuFocusTargetmeant both "which bar element" and "suppress its dwell".Both are ownership bugs.
Root cause
TvShellFocusStatecarries ten mutable fields and none of them says who owns focus. Ownership is inferred by reading several together; every combination is representable, including the meaningless ones. Each fix adds a flag excluding one bad combination, which enlarges the space the next defect hides in.The insight is already in
b44e1d8f's commit message:Correct — and that reason is stored nowhere, so it is reconstructed at each decision point, and each reconstruction is a chance to get it wrong.
Proposal
One value: an owner (
Content/Bar/Panel/ProfileMenu/Modal) plus why focus arrived there. Back routing and dwell eligibility become total functions over it — exhaustively testable without a Compose runtime, and adding an owner or reason breaks compilation at every decision point. That compiler enforcement is what the previous design lacked.#204's two new flags collapse into it:
barFocusFromPanelClosebecomesBar + PanelDismissed, andmenuFocusSuppressesDwellstops existing. The bug where an ordinary Up armed Back-close suppression becomes unrepresentable rather than excluded by a flag.Scope
In:
TvShellFocusState,TvMainShell,TvTopMenuBar, Back/dwell decisions — the 15-edit hotspot.Out: screen-local initial-focus acquisition and the bounded observed-focus policy. That half works and stays.
Not a global focus coordinator — screens keep resolving their own targets; the shell stops guessing what state it is in.
Success criteria
Stated in the doc and measurable: focus
fix:commits per month, edits toTvMainShell.ktper month, andfocus trap suspected/navigation struggleper tester session. If they do not fall, the document was wrong and should be replaced rather than supplemented.Depends on #204 landing first.