docs: design an explicit focus-ownership model for the TV shell - #211
docs: design an explicit focus-ownership model for the TV shell#211RXWatcher wants to merge 7 commits into
Conversation
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>
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 note described barFocusFromPanelClose as a flag Silo-Server#204 adds. Review found it unreachable from production UI and Silo-Server#204 deleted it instead — which is evidence for the note's own argument, not against it: the flag was carried through routing and tests while describing a state the app could not reach. Also records panelHasFocus, the flag Silo-Server#204 did add, as the case this model would keep under a different name: entry INTENT versus observed ownership is a type distinction being expressed as a boolean pair because the type does not exist yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 9 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)
📝 WalkthroughWalkthroughThis design replaces mutable TV shell focus flags with sealed ownership and explicit arrival reasons. It defines exhaustive Back and dwell decisions, focus-entry rules, enforcement gates, migration scope, risks, and success metrics. ChangesTV focus ownership model
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)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 |
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@docs/superpowers/specs/2026-08-10-tv-focus-ownership-model-design.md`:
- Around line 77-88: Add a language identifier such as text to the opening
fenced code block surrounding the focus-state field definitions, while
preserving the existing declarations unchanged.
- Around line 118-130: Replace the unconstrained TvFocusOwnership(owner,
arrival) pair with a sealed hierarchy that represents only valid owner/arrival
combinations, or enforce the allowed pairs during construction with a validation
mechanism. Update consumers to handle the new valid-state model while preserving
compiler-exhaustive handling and preventing combinations such as Content with
PanelDismissed or Modal with Browsing.
- Around line 147-150: Expand the design specification with an
owner-by-arrival-reason truth table before the dwellEligible definition. Cover
every owner, including Browsing, Leaving, PanelDismissed, Restored, and
Explicit, and distinguish Browsing eligibility by the relevant owner state such
as Bar rather than marking all Browsing arrivals eligible.
- Around line 223-231: Clarify the Scope section to distinguish roadmap phases
from this PR’s deliverables: keep screen-local focus acquisition explicitly out
of scope, and state whether the enforcement gate and migrations involving
detail, player, audiobook, calendar, library, TvShellFocusState, TvMainShell,
TvTopMenuBar, and Back/dwell paths are planned phases rather than work included
here.
- Around line 196-206: Revise the adoption analysis around the shared bounded
observed-focus policy and direct requestFocus() calls to define the inventory
rule, denominator, and set overlap. Count unique in-scope files, explicitly
account for files appearing in both sets, and recalculate the adoption
percentage and hold-out count from those values.
- Around line 136-159: Update the Back decision model around TvFocusOwner and
backAction to include whether a panel preview is open while the bar owns focus.
Represent this state in Bar or pass explicit panel visibility to backAction,
ensuring the function can distinguish ClosePanelPreview from MenuBack without
relying on external openPanel state.
- Around line 75-95: Correct the field-count statement in the TvShellFocusState
section: identify ten as the pre-PR `#204` count, or update the merged-state count
to twelve after adding menuFocusSuppressesDwell and panelHasFocus. Keep the
discussion of deleting barFocusFromPanelClose unchanged.
- Around line 21-31: Revise the focus-churn evidence in the design spec to use
comparable time windows: state the exact 90-day query and normalize commit
counts by elapsed days, or explicitly describe the table as raw counts without
claiming a rate or rise. Apply the same measurement definition and normalization
to the success metric near the specified success-criteria section.
- Around line 170-176: Update the focus ownership model around “Failure must be
loud” to define behavior when a focus claim does not land, including whether the
state is Unowned or Pending and how observed focus remains authoritative. Add
per-owner diagnostics for failed Bar, Panel, ProfileMenu, and Modal claims using
DiagnosticsInstrumentation, or narrow the model’s claim so it only covers the
existing contentEntryFailed(route) signal.
- Around line 212-221: Revise the Enforcement section’s source-test design to
detect every screen-local focus-acquisition path, including direct
requestFocus() calls and wrappers or aliases, rather than only runCatching
blocks. Enforce the existing violations with per-file allowlists or changed-file
deltas so a new violation cannot be offset by removing another, while preserving
the migration baseline.
- Around line 134-145: Update the back-action design around backAction and
TvMainShell.handleShellBack to preserve the route-specific preconditions:
Settings must return before the focus-state decision, and Search input focus
must be handled within DelegateToNav. Document these as explicit outer guards or
extend backAction with the necessary context, while retaining exhaustive
ownership handling.
🪄 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: 72ef6375-acb2-4380-8945-bad712c29d69
📒 Files selected for processing (1)
docs/superpowers/specs/2026-08-10-tv-focus-ownership-model-design.md
| ``` | ||
| menuFocusRequest Int token | ||
| menuFocusTarget TvTopMenuPanel? | ||
| profileFocusRequest Int token | ||
| panelFocusEntryToken Int token | ||
| profileMenuFocusEntryToken Int token | ||
| isMenuFocused Boolean | ||
| profileMenuOpen Boolean | ||
| profileMenuEntered Boolean | ||
| panelEntersFocus Boolean | ||
| openPanel TvTopMenuPanel? | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced block.
Specify text or another appropriate language after the opening fence at Line 77.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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-10-tv-focus-ownership-model-design.md` around
lines 77 - 88, Add a language identifier such as text to the opening fenced code
block surrounding the focus-state field definitions, while preserving the
existing declarations unchanged.
Source: Linters/SAST tools
| sealed interface TvFocusOwner { | ||
| data class Content(val route: String) : TvFocusOwner | ||
| data class Bar(val tab: TvTopMenuPanel) : TvFocusOwner | ||
| data class Panel(val panel: TvTopMenuPanel) : TvFocusOwner | ||
| data object ProfileMenu : TvFocusOwner | ||
| data class Modal(val id: String) : TvFocusOwner | ||
| } | ||
|
|
||
| /** Why focus arrived where it is. Governs Back and dwell, nothing else. */ | ||
| enum class TvFocusArrival { Browsing, Leaving, PanelDismissed, Restored, Explicit } | ||
|
|
||
| data class TvFocusOwnership(val owner: TvFocusOwner, val arrival: TvFocusArrival) | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make invalid ownership states unrepresentable.
TvFocusOwnership permits combinations such as Content(route) + PanelDismissed and Modal(id) + Browsing. A total when does not reject these states, so the claimed compiler enforcement is not achieved. Use a sealed hierarchy for valid transitions or validate construction and define the allowed pairs.
🤖 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-10-tv-focus-ownership-model-design.md` around
lines 118 - 130, Replace the unconstrained TvFocusOwnership(owner, arrival) pair
with a sealed hierarchy that represents only valid owner/arrival combinations,
or enforce the allowed pairs during construction with a validation mechanism.
Update consumers to handle the new valid-state model while preserving
compiler-exhaustive handling and preventing combinations such as Content with
PanelDismissed or Modal with Browsing.
| Two pure functions replace the scattered conditionals: | ||
|
|
||
| ```kotlin | ||
| fun backAction(ownership: TvFocusOwnership, canNavigateUp: Boolean): TvShellBackAction | ||
| fun dwellEligible(ownership: TvFocusOwnership): Boolean | ||
| ``` | ||
|
|
||
| Both are total `when`s over a sealed type, so they are exhaustively testable in | ||
| JVM tests with no Compose runtime, and — the part that matters — **adding an | ||
| owner or an arrival reason breaks compilation everywhere a decision is made.** | ||
| That is the enforcement the previous design lacked: the compiler, not a | ||
| convention. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve route-specific Back preconditions.
TvMainShell.handleShellBack() returns before the focus-state decision for Settings and handles Search input focus inside DelegateToNav (androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvMainShell.kt, Lines 863-911). backAction(ownership, canNavigateUp) does not receive either condition. Document these as explicit outer guards, or add the required context.
🤖 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-10-tv-focus-ownership-model-design.md` around
lines 134 - 145, Update the back-action design around backAction and
TvMainShell.handleShellBack to preserve the route-specific preconditions:
Settings must return before the focus-state decision, and Search input focus
must be handled within DelegateToNav. Document these as explicit outer guards or
extend backAction with the necessary context, while retaining exhaustive
ownership handling.
| `#204`'s flags collapse into this directly. `menuFocusSuppressesDwell` stops | ||
| existing: `dwellEligible` returns false for `PanelDismissed` and `Leaving`, | ||
| true for `Browsing`. The bug where an ordinary Up armed the Back-close | ||
| suppression becomes unrepresentable rather than excluded by a flag. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Define dwell behavior for every owner and arrival reason.
The document defines behavior for Browsing, Leaving, and PanelDismissed, but not for Restored or Explicit. It also states that Browsing is eligible without restricting eligibility to Bar. Add an owner-by-reason truth table before implementing dwellEligible.
🤖 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-10-tv-focus-ownership-model-design.md` around
lines 147 - 150, Expand the design specification with an owner-by-arrival-reason
truth table before the dwellEligible definition. Cover every owner, including
Browsing, Leaving, PanelDismissed, Restored, and Explicit, and distinguish
Browsing eligibility by the relevant owner state such as Bar rather than marking
all Browsing arrivals eligible.
| ### Failure must be loud | ||
|
|
||
| The 08-04 design says observed focus is authoritative. Keep that, and add: a | ||
| claim that never lands emits a diagnostic. `#199` added exactly that warning | ||
| and `#203` is what stops it crashing the app — with those merged, the signal | ||
| exists. `focus trap suspected` and `navigation struggle` already arrive from | ||
| real devices, so we can measure whether this works instead of asserting it. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Model failed focus claims and their diagnostics.
The model has no Unowned or Pending state, although the document says focus claims can fail and observed focus is authoritative. The supplied DiagnosticsInstrumentation.kt only exposes contentEntryFailed(route); it does not cover failed claims for Bar, Panel, ProfileMenu, or Modal. Define the no-owner behavior and per-owner diagnostics, or narrow the claim.
🤖 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-10-tv-focus-ownership-model-design.md` around
lines 170 - 176, Update the focus ownership model around “Failure must be loud”
to define behavior when a focus claim does not land, including whether the state
is Unowned or Pending and how observed focus remains authoritative. Add
per-owner diagnostics for failed Bar, Panel, ProfileMenu, and Modal claims using
DiagnosticsInstrumentation, or narrow the model’s claim so it only covers the
existing contentEntryFailed(route) signal.
| ### Enforcement | ||
|
|
||
| Add a source test that fails on `runCatching { … requestFocus() … }` in | ||
| `androidTvApp/.../ui/screens/`. The repo already uses `*SourceTest.kt` files | ||
| that read source by path and assert on structure, so this is an established | ||
| mechanism rather than a new one. Existing sites are baselined; the gate is that | ||
| the count may only go down. | ||
|
|
||
| That stops new instances appearing while the 36 hold-out files are migrated in | ||
| churn order. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the source gate prevent new violations.
The proposed test matches only runCatching { … requestFocus() … }, but the measured problem also includes direct requestFocus() calls. A new direct call, wrapper, or alias can bypass the gate. An aggregate-count baseline also permits adding one violation while removing another. Enforce all screen-local acquisition paths and compare per-file allowlists or changed-file deltas.
🤖 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-10-tv-focus-ownership-model-design.md` around
lines 212 - 221, Revise the Enforcement section’s source-test design to detect
every screen-local focus-acquisition path, including direct requestFocus() calls
and wrappers or aliases, rather than only runCatching blocks. Enforce the
existing violations with per-file allowlists or changed-file deltas so a new
violation cannot be offset by removing another, while preserving the migration
baseline.
…r#208 invalidated Pre-merge gate caught three internal contradictions, and checking them against main showed the problem was larger than arithmetic. The adoption section was the note's central argument: 8 adopters against 44 direct callers, 18%, 107 runCatching occurrences, therefore "enforcement first, migration second, shell model third". Measured on main at 7245c0f that is no longer true. Silo-Server#208's sweep of 78 sites and its ratchet have taken adoption to 34 files against 24 with a raw requestFocus() — 9 of which use both — so 69%, with 15 genuine hold-outs and exactly ONE runCatching still wrapping a requestFocus. So the note now says that plainly and retracts its own ordering: enforcement exists and adoption followed it, and what remains is the part a sweep cannot do — the shell still infers ownership from twelve flags, and Silo-Server#204's panelHasFocus is the newest instance of a type distinction expressed as a boolean pair. Also corrected: the monthly table (22 and 48, not 26 and 49) and the 90-day split (49 fix, 7 feat of 81) now state their method so they can be re-derived; the field list was missing panelHasFocus and menuFocusSuppressesDwell, which contradicted the Silo-Server#204 paragraph immediately below it; and 49 was being used for three different quantities in the metrics section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the old order The previous commit retracted the enforcement-first ordering in one section and left three others prescribing it: Enforcement still proposed adding the source test, Scope still listed the gate as in-scope and ordered migration before the shell model, and Out still said the observed-focus policy's problem is that it is not used — which contradicts the 69% adoption documented two sections above. Enforcement now records that Silo-Server#208 shipped exactly the proposed gate and that this note proposes none. Scope leads with the shell ownership model, since that is the part a sweep cannot do, and lists the gate as out. Out drops the not-used claim. The adoption, hold-out and runCatching figures now state the greps that produce them, as the commit-subject counts already did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 3 found the retraction still contradicted itself and that most figures could not be re-derived. 'Focus entry is a shell concern' asks for a restorer-placement source test, which read as a contradiction of 'no new enforcement'. They are different checks: Silo-Server#208's ratchet catches silent focus claims, the restorer rule is about where a focusRestorer may live in the composition tree. Both statements now say which check they mean. Every empirical figure now carries the command that produces it, and the churn tables are re-measured rather than inherited: file churn 14/9/7/7/6 and directory churn 126/34/14/16, both counted as focus-SUBJECT commits since 2026-05-13. The month table's method now shows a concrete dated window instead of an elided --since, and the runCatching figures state their grep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Closing. #208 shipped the enforcement gate this note proposed, and took helper adoption from 18% to 69% — so most of what it argued for is done, and the rest doesn't justify carrying a design doc around. The one idea it had left is small enough to state here: |
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.
Supersedes #207. That PR was stacked on the pre-merge version of #206 and
carried its original commit, which has since gained tests, per-answer
publication and targeted revalidation. Rebuilt on merged
mainso it isdocs-only, as intended.
Updated against #204 as merged: the note described
barFocusFromPanelCloseasa flag #204 adds. Review found it unreachable from production UI — the only
caller that armed it was a cascade
onClosethe selector never invokes — and#204 deleted it. That is evidence for this note's argument rather than against
it: the flag was carried through routing and tests while describing a state the
app could not reach.
panelHasFocus, which #204 did add, is recorded as thecase this model would keep under a different name.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Q9UyH5fvug685dzUFLtdpW
Summary by CodeRabbit