fix(tv,phone): focus ownership, return focus, and voice search - #168
Conversation
Three screens whose content arrives asynchronously treated "focus was
requested" as "focus was acquired". Their first request lands while the
lazy row or grid is still being placed, which is exactly when it gets
rejected, and the rejection was then latched permanently:
- The server list looped on runCatching { requestFocus() }.isSuccess.
requestFocus returns Boolean, so a plain false rejection arrives as
Result.success(false) — still isSuccess — and the retry loop exited
after its first failed attempt.
- Collections and Collection Detail ignored both the exception and the
returned Boolean and set a one-shot flag unconditionally. Later
placement could not retry, and Collections additionally told the shell
that content focus had succeeded when nothing had been focused.
All three now share one adapter over the Series A observed-focus policy,
which retries within the acquisition budget and reports only observed
acquisition. The shell handoff fires solely on success.
Anchoring is keyed on the first item's stable identity and carries no
additional "already acquired" latch. That is a trade, not a free win. With
such a latch, re-entry sequences — the first item going away and coming
back, or a different key exhausting in between — suppress the request
while nothing holds focus, which is the permanent no-focus state this
change exists to remove. Without it, those same re-entries anchor even
when focus legitimately sits outside the content root, such as in a
confirmation dialog, and pull it back. A dead D-pad is the worse failure,
so it loses. Resolving it properly needs the modal focus-ownership
contract in Series C, since the adapter cannot know a modal owns focus.
Acquisition is observed as hasFocus on the content root, so focus landing
on any item inside the content satisfies it, not the first item
specifically. That is the property worth having, given the failure being
prevented.
A viewer already inside the content is left alone: focus is checked before
the first request rather than a frame into the retry loop.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both shared phone field families installed KeyboardActions(onAny = {
onImeAction() }) unconditionally, against a default callback of {}.
Installing an action handler takes ownership of the IME event, so with the
default in place Compose's own handling was replaced by a no-op: pressing
Next ran an empty lambda and left the cursor where it was.
That is every multi-field form on phone — Login, Signup, Setup, Create
Profile, Edit Profile, Invite Claim — where advancing between fields
needed a tap because the keyboard's own Next button did nothing. Only the
five call sites that pass a real submit callback ever worked.
The callback is now nullable and the handler installed only when one is
supplied. Leaving the slots null hands the action back to Compose's
KeyboardActionRunner, whose defaults move focus for Next and Previous,
close the IME for Done, and do nothing for Go, Search and Send. Fields
that pass a submit callback keep their behavior and invoke it once.
The unit test covers the wiring only: that no handler is installed without
a callback, and that a supplied one owns every action slot so nothing
falls through to a default the caller did not ask for. Traversal itself is
Compose's behavior and would need a Compose UI test with two fields and
performImeAction to assert end to end.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Android TV does not take the IME down when the surface that raised it leaves composition, so it floats over whatever screen comes next and keeps consuming the D-pad. TvSearchScreen and TvTextInputDialog each carried their own copy of the disposal fix; the two surfaces that copied only the focus-and-show half did not: - TvCreateCollectionDialog showed the keyboard after focusing the name field and never hid it — not on Done, dismissal, successful creation, or disposal. - TvServerSetupScreen shows the keyboard on its URL field and likewise never dismissed it. Beyond the audited finding, but the same defect, and leaving a known instance in place to match a report is not a reason. The disposal is now one shared composable rather than a comment repeated at each site. The create-collection dialog also had no IME inset handling, so the keyboard could cover the dialog it belonged to. imePadding alone would have been inert here: a Dialog gets its own window, which by default fits system windows itself and reports no IME inset to the modifier, so the padding needs decorFitsSystemWindows = false to receive anything. A policy test walks the TV sources and asserts that a file taking the keyboard controller and showing it also uses the helper. It is a source check: it catches the copy, not every conceivable way of raising the IME, and it is per file rather than per composable, so one helper call blesses everything in that file. Verified to fail when the disposal is removed from the collection dialog. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The profile screen reloads on every ON_RESUME so a profile edited on a pushed screen is reflected on return. Initial focus and refresh shared one effect keyed on the whole profile list, which unconditionally requested the first card — so every reload overrode where the viewer actually was. Editing the fourth profile and coming back, or deleting it in manage mode, dropped focus onto the first tile. Focus is now decided by a pure function of the previous and current ID lists, the focused ID, and whether the screen has ever anchored: - first arrival anchors the first tile, so the D-pad has a home; - a profile that survives keeps focus, and is re-requested so a reordered tile carries focus with it; - a deleted profile falls to whatever took its index, or the new last tile; - no focused profile means the refresh moves nothing; - an empty list anchors nothing. That needs per-profile identity, so the grid takes a requester per profile ID and reports which one holds focus, replacing the single first-card requester. Requesters for deleted profiles are pruned rather than retained for the screen's lifetime. Reporting focus only on gain would have left the last focused tile named forever, so a refresh would re-request it after the viewer had moved on. The grid therefore reports null when focus leaves it, and the Add tile — which lives inside the grid, so a container-level check alone would miss it — reports null when it takes focus. Two ordering details the retry depends on. The anchored flag is set only once focus has actually landed: setting it up front meant a second list arriving mid-retry cancelled the first pass and left the replacement believing the screen was already anchored, so it never anchored at all. And the retry abandons its target once the viewer focuses a different tile, rather than fighting them for the rest of the relocation budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The token route deep-links in and starts its lookup automatically, which disables Check while it runs and enables Approve/Deny when it resolves. None of that changes completedStatus — and completedStatus was the sole key of the initial-focus effect. So focus was requested once, against a control that was disabled at that instant, and never asked again. The action panel could sit with no focus owner at all, leaving the D-pad dead on a screen whose entire purpose is approving or denying. Focus now follows a pure function of the states that actually gate eligibility, and the effect is keyed on the resulting action, so every loading, resolved, error, submitting and completed transition re-acquires: - completion offers Done; - a resolved lookup offers Approve, which outranks Check because approving is what the viewer came to do; - otherwise manual code entry, if this is not a token route; - otherwise Check, but only while it is enabled; - and nothing at all while a lookup or a submission is in flight, which is a legitimate wait rather than a target. Deny is never the default: it sits beside Approve, one press away, and defaulting focus to the destructive choice would be wrong. One requester was previously attached to three different buttons across mutually exclusive branches. Each action now owns its own, so the request cannot land on whichever node happened to bind it last. Acquisition reuses the content-focus adapter, so it retries through placement and does not pull focus back if the viewer is already on a control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A fresh Up on the first populated shelf returns focus to the controls, and the handoff stays in flight until the controls actually take focus. For those frames the shelf still owns focus and still reports its own index — so neither guard held: - shouldReturnCalendarFocusToControls goes false the instant the return starts, because it requires !isReturningToControls; - the repeat freeze below it only matched a null shelf index. The held key therefore fell through to geometric movement, and the same press that began the handoff walked straight back into the content it was leaving. Repeats are now frozen whenever a return is in flight, without consulting the shelf index, since the index cannot distinguish "handoff finished" from "handoff still settling". A fresh press during an in-flight return is left alone: that is the viewer acting again, not the tail of the press that started it. Verified to fail without the new branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sheet's own doc comment called it focus-trapped, and it was not. Its only boundary was focusGroup(), which prioritises traversal inside the group but does not cancel a focus search at the group's edges. Browse keeps its header and grid composed behind the scrim, so pressing toward an edge from a boundary chip walked spatial focus straight out of the sheet and onto controls behind the dimming — visible focus on a page the viewer believes is blocked. The boundary now cancels the focus search on exit, matching the idiom the shell, the audiobook overlay and the player HUD already use. Cancelling up/down/left/right on the content root instead would have governed movement between the sheet's own chips as well, trading an escape for a dead D-pad inside the modal. Dismissal was the other half. Back only called onDismiss and left the focus system to pick a geometric successor, which is rarely the control that opened the sheet. The Filters pill is now a restorable opener and receives focus back when the sheet closes. Restoration is driven from the screen rather than from inside the sheet: the exit animation keeps the sheet's nodes alive after visible goes false, so anything hosted within it cannot outlive its own dismissal. It also observes the opener rather than trusting the return value of a request, since the pill is not focusable again until the animation has finished tearing the sheet down — an unobserved retry would either stop early or keep firing for the whole budget after focus had already landed. Acquisition on open now uses the same observed retry as the other async surfaces, instead of one fire-and-forget request made while the sheet was still sliding in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit modelled eligibility on canSubmit, assuming it meant
"the lookup resolved and there is something to approve". It does not.
DevicePairingUiState defines it as
!isSubmitting && (token or code is non-blank)
so on a token route it is true from construction, before the automatic
lookup has returned anything. Focus therefore targeted Approve during the
lookup — pointing the viewer at a decision about a device whose details
had not arrived — and the accompanying tests asserted state combinations
the view model cannot produce, so they passed while describing fiction.
The resolved lookup is the actual signal, so it is now an explicit input.
During a token lookup nothing is focusable, which is honest: Approve is
not yet meaningful and Check is disabled while loading. A failed lookup
keeps the identifier and falls to the re-enabled Check. Entering a code
makes canSubmit true immediately, so manual entry stays the target until
the lookup resolves.
Tests now mirror reachable view-model states only, and say so.
Not addressed here: Approve and Deny are enabled on canSubmit alone, so
they are pressable during a token lookup. That is the same conflation in
the enablement rather than the focus, and it is a behaviour change rather
than a focus fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The audiobook panels are in-window overlays: the player stays composed and focusable behind the scrim. Each panel hand-rolled its own single unobserved requestFocus() — Speed and Skip fired at composition before layout, Chapters and Sleep slept a fixed 100ms and asked once. Losing that race left the panel open with focus still on the transport pill behind it, where Select operated the hidden player. The panel's exit = Cancel containment cannot help, because it only holds focus that already got in. Three coupled changes, in dependency order: - Acquisition moves into TvAudiobookOverlayScaffold, retried until observed. initialFocus is required rather than optional, so a panel that cannot own focus is a compile error. - The covered transport buttons and pills leave the focus graph while a panel is open, via a new tvFocusSuppressed. It is applied on each leaf's own modifier chain: a focus target resolves properties by walking up until the first ancestor that is itself a focus target, so container-level suppression can be swallowed by an intervening focusGroup, TV Surface, or focusable. (focusGroup is not that mechanism — it deactivates its own target through Focusability.Never, which is why its children stay focusable.) - Restoration on panel close becomes observed, because suppression means there is no longer anything else holding focus if a request lands early. The ordering matters: TvControlEnablement already records that Android TV does not re-home focus when the focused node stops being focusable. Suppressing without guaranteeing acquisition would trade an escape bug for a dead D-pad, which is worse. Acquisition is retried, not guaranteed, so failure is handled rather than assumed away. The ladder is: request the named row until observed; traverse into the panel and confirm that landed (moveFocus is a global directional move, so a true return proves focus moved, not that it moved into this panel); hand the player back and acquire play/pause; and finally close the overlay, which changes the tree instead of re-asking the same question and terminates the escalation. Degrading to the original escape bug is survivable; a dead screen is not. Two panels held no focusable at all and would have been stranded by the suppression: the About panel and the AI Translate empty state. Both gain a Close row. AI Translate also replaces an unbounded `while (!overlayHasFocus)` loop — which re-requested a FocusRequester attached to no node every 60ms for as long as the dialog stayed open — with the shared adapter, extended with reacquireKey/enabled so a dialog that swaps its own body re-acquires. Its key tracks the shape of the focus graph, not the phase: track availability can change under an open dialog and swap the empty state for the picker form without the phase moving. Row enablement is deliberately excluded — a quota-exhausted submit row stays focusable and only refuses to act, so it cannot strand focus. Also: a runtime error arriving under an open panel left it floating over TvErrorScreen, which has no actions, so closing it had nowhere to send focus. The panel now closes with the content it belonged to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DevicePairingUiState.canSubmit was `!isSubmitting && (token or code non-blank)`, and both pairing screens gated Approve and Deny on it. A silo://device?token=… deep link arrives with its token already set and starts its lookup automatically, so Approve was live from the first frame — before the server had returned the device name, IP hint or match code. Those details are the entire content of the decision and render only once the lookup resolves, so the viewer could grant access to a session they had no way to see. The error path clears the lookup and keeps the identifier, so Approve also stayed live on a request the server had just called invalid or expired. Replaced with `canDecide = lookup != null && !isSubmitting`, tested in shared: the deep-link and failed-lookup cases fail against the old predicate. Deny is gated the same way deliberately. Rejecting junk faster is worth something, but an unsolicited deep link could otherwise be denied irreversibly before the viewer saw which request it was, and an invalid or expired request needs no denial. On TV the two halves of "disabled" are separated. No lookup means the decision cannot apply at all, so the buttons leave the focus graph rather than standing as dead stops; a decision in flight is momentary, so those stay focusable and merely refuse to act, which keeps focus from being dropped mid-submit. That exposed a promise the codebase was not keeping. TvControlState has carried a `focusable` flag since Series A, and call sites passed it to the TV component's `enabled` parameter — but `tvClickable`, the shared basis of TV Material's clickable Surface and therefore of Button, calls focusable() with its default `enabled = true` and never forwards the component's own `enabled`. A disabled TV button is still a focus stop. Every call site until now used TvControlState.transient, where focusable is always true, so nothing had tested it. tvControlSemantics now applies the exclusion itself, so it cannot be forgotten again. tvPairDeviceFocusTarget loses its isLoading/isSubmitting parameters and its nullable return with them. It answers which control deserves focus, not which is focusable; every incomplete state renders Check and every completed one renders Done. Its previous null was justified by the same mistaken belief that a disabled Check had left the focus graph. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Zero call sites. Every password input in the app — Login, Signup, Setup, Invite claim, Admin user edit — is on the newer Aurora design system and composes its own masking and reveal toggle over AuroraTextField, while this one was built on the older OutlinedTextField/AuthColors pair. It was a leftover of the pre-Aurora auth design, not a component anything had migrated to yet, so adopting it would have been a visual regression rather than a consolidation. No password input loses masking. The rest of AuthComponents is still in use: SiloTextField and SiloButton by the profile screens, AuthStage / SiloLogo / AuthErrorBanner by the auth and profile screens. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request updates Android IME handling, Android TV focus and return restoration, voice search, playback audio reconciliation, identity-scoped navigation, profile commits, notification attribution, and asynchronous state coordination. ChangesAndroid IME and TV focus
Playback and identity state
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TvScreen
participant FocusUtility
participant FocusRequester
participant FocusNode
TvScreen->>FocusUtility: start keyed focus acquisition
FocusUtility->>FocusRequester: request focus
FocusRequester->>FocusNode: attempt acquisition
FocusNode-->>FocusUtility: report observed focus state
FocusUtility-->>TvScreen: invoke success or failure callback
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 2
🧹 Nitpick comments (1)
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookOverlay.kt (1)
50-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
camelCasefor focus-budget properties.These properties start with
Tv, so they do not usecamelCase.
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookOverlay.kt#L50-L61: Rename the threeTvAudiobookPanelFocus*properties to names that start withtv.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.kt#L92-L95: Rename the twoTvAudiobookTransportFocus*properties to names that start withtv.As per coding guidelines, use
camelCasefor Kotlin functions and properties.🤖 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/screens/audiobook/TvAudiobookOverlay.kt` around lines 50 - 61, Rename the three focus-budget properties in androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookOverlay.kt lines 50-61 from TvAudiobookPanelFocus* to tvAudiobookPanelFocus*, updating all references. Also rename the two TvAudiobookTransportFocus* properties in androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.kt lines 92-95 to tvAudiobookTransportFocus*, updating all references to follow Kotlin camelCase conventions.Source: Coding guidelines
🤖 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/audiobook/TvAudiobookPlayerScreen.kt`:
- Around line 242-276: Update the panel-focus failure handling in the focus
acquisition effect so that when panelFocusFailed becomes true, activePanel is
immediately set to AudiobookPanel.None before transport focus restoration can
run. Preserve the re-keyed LaunchedEffect behavior so it then restores focus to
playPauseFocus with the overlay removed.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/DevicePairingViewModel.kt`:
- Around line 36-37: Update DevicePairingViewModel’s decision and lookup flow so
starting decide() invalidates or cancels the active lookup and stale lookup
results cannot update state after a decision. Track lookup generations and apply
a result only when it matches the current generation, preserving successful
approval/denial status and errors. Add a test covering approval and denial
completing before an earlier lookup.
---
Nitpick comments:
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookOverlay.kt`:
- Around line 50-61: Rename the three focus-budget properties in
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookOverlay.kt
lines 50-61 from TvAudiobookPanelFocus* to tvAudiobookPanelFocus*, updating all
references. Also rename the two TvAudiobookTransportFocus* properties in
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.kt
lines 92-95 to tvAudiobookTransportFocus*, updating all references to follow
Kotlin camelCase conventions.
🪄 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: 61712e1e-77eb-416f-a26e-968e62dd6979
📒 Files selected for processing (40)
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/components/SiloKeyboardActions.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/components/aurora/AuroraChrome.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/AuthComponents.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/DevicePairingScreen.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/components/SiloKeyboardActionsTest.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvDialogInitialFocus.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvFilterSheet.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvStockImeLifecycle.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvTextInputDialog.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvContentInitialFocus.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvControlEnablement.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvModalFocusOwnership.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvPairDeviceFocusTarget.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvProfileFocusTarget.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookBookmarksPanel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookChaptersPanel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookOverlay.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookSkipIntervalPanel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookSleepPanel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookSpeedPanel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookTransportRow.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/auth/TvPairDeviceScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/auth/TvServerSetupScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/browse/TvBrowseScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/calendar/TvCalendarScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/collections/TvCollectionDetailScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/collections/TvCollectionsScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/collections/TvCreateCollectionDialog.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvAiTranslateDialog.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/search/TvSearchScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/servers/TvServerListScreen.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/components/TvStockKeyboardPolicyTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvContentInitialFocusTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvPairDeviceFocusTargetTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvProfileFocusTargetTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/calendar/TvCalendarFocusRoutingTest.ktshared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/DevicePairingViewModel.ktshared/src/commonTest/kotlin/org/siloserver/silo/viewmodel/DevicePairingUiStateTest.kt
…supersedes Both found by review of Silo-Server#168. The audiobook panel could end up open with the transport focusable behind it. When a panel reported that it failed to take focus, the player was unsuppressed so focus had somewhere to go — but the panel and its scrim were still drawn, so D-pad and Select landed on controls the viewer could not see. The escalation that closes the overlay only fires when the transport ALSO fails to take focus, so the case where it succeeds went unrescued. A panel that cannot hold focus now closes, and the re-keyed effect acquires against an unobstructed player. Device pairing let a stale lookup overwrite a completed decision. canDecide stays true while an existing lookup refreshes — the previous result is left on screen deliberately rather than blanked — so approving mid-lookup is ordinary. If that lookup landed last it could paint a lookup error over a successful approval, or clear the error a failed decision had just reported, leaving someone believing the opposite of what happened. A decision is the more authoritative event, so starting one retires any lookup already running, and a retired lookup releases its loading flag and says nothing else. Tests cover both orderings and fail without the guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hiding awaitState polled for five real seconds. That mechanism is right — the Ktor engine behind these repositories completes on its own dispatcher, so the work is on real threads and the test scheduler can neither see nor advance it; draining the scheduler instead returns before any response arrives, which is worth stating because it is the obvious "fix" and it does not work. The budget was the problem, not the mechanism. Five seconds is ample on an idle machine and not always ample with several test modules sharing cores, so the failure was load-dependent rather than logical. It is now generous enough that only a hang trips it, and it says what it was waiting for. Properly deterministic needs the engine dispatcher injectable the way SectionRepository's already is, which is a production change rather than a test one. The flake was also masking a real failure, exactly as feared: it aborted `gradlew test` before :androidTvApp:testReleaseUnitTest ever ran. Those Robolectric suites cannot pass in release — they need the test ComponentActivity in the merged manifest, and that dependency is deliberately debug-only so it never reaches the release APK. Unit tests are not minified, so running them twice exercised no different code; the release variant's unit tests are now disabled rather than left permanently red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TV surfaces restore focus after a detail page by saving two numbers. TvSkylineSectionFeed keeps returnRowIndex/returnItemIndex; the shell holds a launch-card FocusRequester for Home and For You and nothing at all for the rest. Indices survive a refresh syntactically and address different content, so coming back after Continue Watching reordered, after the episode just finished left its row, after a grid was re-sorted, or after the process was recreated puts focus on something the viewer never chose. Identity here is the item qualified by its section. The pair is the occurrence, which matters because feeds carry the same title in several rows at once: the copy in Continue Watching is not the copy in Recently Added, and treating them as interchangeable throws focus across the feed to a card nobody touched. Indices stay, demoted to what they are — a coordinate for the fallback once the occurrence is genuinely gone. Absence has to be proven before it is acted on. Every flat surface here paginates, and an item on page four is missing from page one exactly as a deleted item is, so sections carry completeness, the section list carries its own, and an unproven absence resolves to Pending rather than spending the target. Pending's terminal path is part of the contract too: a caller that has exhausted its wait asks again with treatAbsenceAsFinal instead of lying about completeness or rebuilding the fallback eight times over. Resolutions carry what they resolved to, not only where. Data can change again between resolving, scrolling, attaching a requester and requesting focus, and the identities are what let a caller confirm it landed on the thing it asked for. Cross-row following is opt-in. An item leaving its row is indistinguishable from a copy that was always elsewhere, so overlapping feeds keep the positional fallback and only surfaces with disjoint sections opt in. Section ids must be unique and stable. The no-stall rule treats a present, finished launch section as the final word, which only holds if no later section can arrive bearing the same id; duplicates are handled deterministically anyway so a data bug degrades predictably rather than moving focus on every refresh. The Skyline adapter answers the one question completeness actually asks — can more arrive in this row — which splits a Skyline feed in two. A row with cards is complete however large its totalCount, because it was capped rather than paged and nothing will fetch the rest; a row with no cards and a non-zero total is a placeholder hydrateHomeSections has yet to fill, and calling that complete would spend the target moments before the real row arrived. No surface is wired yet. This lands alone because eight of them will depend on its shape, and five review rounds against it turned up a duplicate treated as the launch card, pagination read as deletion, a flat surface that could never match its own section, and a resolver that discarded the identity it had just established — each of which would otherwise have been copied eight times and unpicked from eight places. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he index First surface onto the return-target contract. The feed saved a row index and an item index and, on the way back, retried until `focusedRowIndex == rowIndex && focusedItemIndex == itemIndex`. That confirms coordinates and never content: it reports success for whatever now occupies the slot. Come back after Continue Watching reordered, after the episode just finished left its row, or after the process was recreated from fresh server data, and it lands on something else and calls it done. The launch card is now recorded by identity — its row and its content id — and resolved against the rows the feed renders. Landing is confirmed the same way, so a card that moved is followed and a card that is merely in the right position is not mistaken for it. The bounds checks the three ladders each carried are gone: the resolver cannot return an index it did not just read. Three near-identical ladders collapse into one driver, which is where the rest of this had to go. It re-reads the resolution every attempt. A refresh that keeps the same first row does not restart these effects, so a ladder that captured its destination would keep driving at a row the feed has since moved while the success check watched the new one — unable to succeed, and for the shell ladder the request token was already spent, so nothing retried. The vertical band is re-scrolled when the resolved row changes rather than once at the start, and the row requester is chosen from the fresh index rather than a captured first-row id. It cannot be hijacked. Every focus gain re-arms the return target, and focus lands on the wrong card first often enough that these ladders exist for it — so an intermediate card's callback would overwrite the armed identity and the ladder would then confirm success against content the viewer never opened. Re-arming is suppressed while a restoration runs, counted rather than flagged because two ladders can be live at once, and read through its state at call time rather than captured, which left a window either side of every change. The refresh-and-removal path is guarded too; it was a second door to the same bug. It cannot outlive its trip. Following the current resolution means an old ladder would otherwise pivot onto a newly clicked card, find it already focused, and retire the new trip's restoration before it began. Each arming bumps a generation, ladders abandon once it moves, and only the owner may retire the target. The row now also receives a restore request, so it scrolls its own LazyRow to the resolved card. Passing only the index was survivable while the index could not change; with identity resolution a relocated card can sit outside the composed window, leaving the requester unattached and every retry doomed — the contract's obligation to scroll the destination into composition, unmet at its first call site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second surface onto the return-target contract, and the first paginated one. The grid saved a single item index and clamped it to the current count on the way back, so a re-sort, a filter change or a page arriving above the card all pointed it at different content. Its own test asserted "17 comes back as 17", which is the behaviour being removed. The launch card is recorded by identity now, and confirmed the same way before restoration reports success. Pagination is what makes this surface different from the row feed. An item on a later page is missing from the loaded ones in exactly the way a deleted item is, and the old code could not tell those apart — it settled on whichever card the loaded pages happened to end with. The resolver answers Pending instead, and this screen honours both halves of the obligation that comes with it: it asks for more pages, and it stops asking. Bounded twice over, because the two bound different things — a six-second clock for what the viewer experiences, and a request ceiling for how hard a struggling endpoint gets pushed. Waiting on a page is a two-phase handshake. loadMoreBrowse launches its work, so waiting only for "not loading" can succeed instantly against the state from before the request; a loop would then fire every request it had before the first fetch marked itself active, and the view model would accept them all because it also still saw idle. So the wait watches for the load to become active, then to settle — and reports which of those it actually observed, because the caller must not blame a request it never saw start. That distinction is the difference between "this page failed" and "this page has not begun yet", and the flags alone cannot tell them apart. Failure is judged from outcomes rather than from state read at a moment. Growth is progress whatever an older error still says; a load that finished with nothing to show and left an error behind is a real failure, including when its message matches the previous one — which comparing error values would read as nothing having happened. Landing is gated on the requester, not on the layout. A card can be laid out while its modifier still carries the previous binding, so the card now reports which identity its restore requester is bound to and acquisition holds until that matches. The destination is published as one value, because index and identity updated from different sources drifted: a page landing mid-attempt moved the requester onto the real card while the watcher still waited for the fallback, so focus arrived exactly where it should and was recorded as a failure. Restoration latches whether or not it succeeds, because nothing re-keys the effect and "try again later" would mean never — but it only tells the shell that content took focus when it actually did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Behaviour-preserving. The library grid's restoration now lives in rememberTvFlatReturnRestoration, and the screen keeps only what is genuinely its own: the header-to-grid index conversion, the requester, and three callbacks. It sheds 288 lines for 22. The reason to move it is that four more surfaces need it — personal lists, people, Collections, Requests — and it took nine review rounds to get right on one screen. Every one of those rounds was the same class of mistake: inferring an outcome from a state snapshot, or letting two sources of truth drift. Written out four more times it would be got wrong four more ways, each subtly different and each found by a viewer rather than a test. What the helper had to add to be reusable rather than single-use: The surface key is a String required to be stable and injective, saved WITH the target and checked on restore. rememberSaveable does not validate restored values against its inputs, so a process returning while composing a different tab would otherwise adopt the previous surface's target; and Any?.toString() is not a partition — 1 and "1" collide, null and "" collide, and a default toString can change across process recreation and silently discard a valid target. The saveable state is the holder's actual backing rather than something mirrored into it. Mirroring updated on a later recomposition, so a click that navigated first carried the previous trip's target — defeating the "click always arms" rule that exists precisely because a card focused during restoration never re-arms. Changing the surface key now resets everything, where the screen reset the target but kept the completion latch. That split was unsound on its own terms: had its tab branches not disposed the subtree, a second tab would have found the latch set and never restored focus at all. It also records one thing it deliberately does not do. When a caller changes the surface key without recreating its item content, the cards never re-report their attachment, the holder waits for an acknowledgement nobody will send, and restoration is lost silently. A blind last-resort request was tried and reverted: "no card reported focus" is not "nothing has focus", and these surfaces also hold sort and filter controls, genre chips and an A-Z rail — the rescue could steal legitimate focus seconds after the viewer arrived, which is worse than what it was salvaging. Making it sound needs an authoritative signal only a caller can supply, so it stays a precondition, with the reasoning in the file so the next person finds the answer instead of shipping the unsafe version. Search is excluded and says so: it mixes library results with request-provider results in separate containers with unrelated identifier spaces, so it needs real section ids and namespaced item ids rather than the flat path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Series D, applied to the first of the flat surfaces. Collections, personal lists, People, Search and Browse all render through TvCatalogGrid, and none of them restored focus at all — returning from a detail page always landed on the first card. The card-level plumbing goes in the grid once: the requester, the attachment acknowledgement the restoration contract requires, and the focus callback. Defaults leave the existing first-item behaviour exactly as it was for callers that do not opt in. Collection Detail is the first caller, replacing its plain initial focus. That covers first entry too, where nothing is recorded and the resolution is the first item — though not identically: the old adapter re-armed when the first item's identity changed and this runs once, which is dormant here only because loading appends rather than replaces. Attachment is tracked by identity, not by a flag. A flag cannot express ownership, and paging moves the restore index while both the old and new cards are briefly composed: if the successor attaches before the predecessor disposes, the predecessor's teardown erases a live attachment, and the grid and its caller then agree with each other while both are wrong. Disposal now clears only what it still owns, and the effect is keyed on the callback as well as the item so a change of owner re-announces rather than only reporting the eventual null. The focus restorer no longer names a requester nothing is holding. That was already possible before any of this — its fallback was the first-item requester whether or not item 0 was still composed — and a deep restore target makes it easy to hit, since scrolling there disposes item 0 on the way. The gain is smaller than it looks and the comments say so: an unattached requester returns false and Compose continues with normal entry, the same place the default reaches. The case that actually matters is a requester attached to the WRONG card, which is what the identity ownership prevents. Two limits are recorded rather than papered over. The fallback is chosen during composition while attachment is known only after it, leaving a one-recomposition window in both directions; and the restore index is a position, so a list that re-sorts in place after resolution will follow the index onto a different item. Both are stated at the parameters, with what a caller must do about them. The remaining flat surfaces are deliberately not wired here. None is a copy of this one: People enters on its filter chips rather than the grid and needs restoration to stand down when nothing is recorded, personal lists replace their contents on resume refresh so the one-shot latch becomes observable, and Requests uses its own LazyColumn with rows whose identity is not what they navigate to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Favorites, watchlist and history refresh on every resume, which is exactly when a viewer comes back from a detail page. A page requested before that refresh describes a list the refresh then throws away: when it lands it appends items fetched at offset N on top of a list that is now page one, leaving a hole where the middle used to be. Gating the triggers cannot fix this on its own. The page is already in flight when the refresh starts and nothing cancels it, so loadMore() rejecting a visible refresh only closes the case where the viewer had not already scrolled. A restored deep scroll position sits right at the paging threshold, so that case is the common one, not the rare one. So the check moves to where the answer is knowable: every replacing load bumps a generation, and a page verifies it after fetchPage returns. A superseded page drops its items and its error — a stale request's failure is not this list's failure, and showing it would put an error banner over content that loaded perfectly well — while still releasing its own loading flag, because the request really has finished. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Wire People and the personal lists to the shared flat-surface restoration, and teach it the difference between a list being EXTENDED and a list being REPLACED. Folding "refreshing" into isLoadingMore looked like it covered this and did nothing at all: isLoadingMore is only consulted once resolution has already come back Pending, and a stale multi-page list still CONTAINS the target, so it resolves Exact and that check is never reached. Restoration was scrolling and acquiring against a list about to be discarded — the card vanishes mid-acquisition and the attempt fails, or focus lands on a card removed a frame later. isReplacingContent therefore gates the FIRST resolution instead. It waits for quiet and then watches a window for a restart, because proving quiet and observing one false reading are not the same thing: a resume refresh is dispatched a frame or two after recomposition, so a check on arrival sails straight past it. If a replacement outlives the budget, restoration retargets to the first item rather than the recorded position — a reload produces page one, so index zero is the one place a replacement cannot invalidate, and standing down would leave the surface with nothing focused. People needs its filter in the surface key, and the key() that gives the helper the card recreation it requires also destroys the focused chip. Every filter change now refocuses the selected chip. An earlier version armed this from the click so an async fallback to All could not steal focus from a poster — but the key change has already destroyed that poster by the time the effect runs. There is no focus left to protect, only focus to lose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last flat surface. Rows are identified by request id rather than by what they open: a row with a library item opens that item while every other row opens the request detail, so two rows can share a destination and keying on it would send focus to whichever came first. The projection is built from the filtered list, because that is what is rendered — the unfiltered one would put every index in a different coordinate space from the rows these indices address. Restoration is the only claimant for entry. The separate first-item requester it replaces was unattached whenever restoration owned row zero, and it reported a content handoff to the shell that it had not made; the handoff now happens only on confirmed focus. Nothing here paginates, so the page hunt never runs. The manual refresh still replaces the list, so it reports that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four things, all found by review rather than by running anything. The generation guard only worked for one of the two orderings. loadMore() read an idle list, then refresh() bumped the generation before the paging coroutine ran, so the page captured the NEW generation, looked current, and appended at an offset belonging to the list the refresh had replaced. Offset, generation and loading flag are now claimed synchronously with the caller's own check, which is what makes that check mean anything. A superseded load cleared flags it did not own. Only paging clears its own; isLoading and isRefreshing belong to whichever replacement superseded it, and clearing them early reported a load as finished while it was still running. The replacement wait used two subscriptions — wait for quiet, then watch for a restart — and a complete pulse between them slipped past unobserved. One subscription now, with the quiet timer restarting on every change. And the timeout path retargeted to the first item of the OUTGOING list, which was unsound: a replacement can reorder it, empty it, or drop that item entirely, so acquisition would confirm against an identity that no longer meant the same thing — and the real target had been overwritten in saved state where no later entry could retry it. It now falls through and resolves live. A replacement landing mid-flight fails the identity check and ends without a landing, which is worse than restoring but not incorrect. The personal grids also still carried the first-item claimant already removed from Requests: unattached whenever restoration owned index zero, and reporting a content handoff to the shell that it had not made. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first sectioned surface, so it uses the resolver primitives directly rather than the flat helper: the day is the section, and the item's own contentId is its identity. Not detailContentId — several episodes of one show share a destination, and keying on where a card GOES rather than what it IS would send focus to whichever the week listed first. Recording is click-only, and that distinction matters here in a way it did not on the pushed routes. Collections, People and Requests are reached by navigation, so a saved target can only have come from a trip. Calendar is a root tab: browsing a card, switching to Home and re-selecting Calendar would be indistinguishable from a detail return, and restoring there would quietly defeat the shell's documented reset to the controls. Process death while merely browsing now forgets the position, which is the lesser cost. The return signal is the lifecycle rather than composition identity. "Was I recreated" answers a question nobody asked — a Back pressed during the outgoing transition can leave this screen composed, and the target would then sit unconsumed until some later, innocent tab selection picked it up. Calendar already had a shell entry handoff that scrolls to the top and takes the controls, and the shell bumps that token on every Calendar selection, Back included. Restoration claims the token before driving and releases it on every path that does not land, so there is exactly one claimant and the loser stands down deliberately rather than by accident. Landing is confirmed by watching focus HOLD the target, not by having asked for it. requestFocus() can be dropped by an unattached requester or a rolled back transaction and says nothing either way, and focus merely passing over the card on its way elsewhere is not an arrival. onInitialContentFocus fires only on a confirmed landing; reporting it early would leave the shell believing content had focus while nothing did, with the top menu suppressed behind it. Resolution waits for refreshing as well as loading. FollowAcrossSections and treatAbsenceAsFinal are both claims about a final snapshot, and a refresh keeps the week's seven dates while replacing everything inside them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Search is the one surface holding two different kinds of thing at once: library items in the grid, and requestable titles in the footer row beneath it. A requestable title already in the library carries BOTH identities, so an un-namespaced projection could match the library twin of the request card the viewer actually opened and land focus in the wrong section entirely. The media type is canonicalised before it is encoded. The rendering pipeline already accepts case and whitespace variants and treats "audiobooks" as "audiobook", so the same card can arrive spelled differently across two responses; encoding it raw would give one card two identities and lose the return whenever the spelling changed under it. Request completeness is modelled here rather than left to the driver. "This row does not paginate" does not mean its contents are final: request search CLEARS its results when a query starts and installs the response later, so there is a window where the row is empty and simply has not answered yet. Resolving then would read absence as final and consume the target on a card that was about to come back — the same mistake as restoring against an unfinished refresh, and putting it in the projection is what stops it being forgotten at a call site. Projection only; nothing drives focus from it yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The flat restoration recorded which card gained focus and never recorded one losing it, so focusedItemId meant "the last card that was focused" while every reader treated it as "the card that is focused now". That is not a cosmetic difference. Acquisition compares the target against that value BEFORE issuing any request, so if the target was the last card reported and focus has since moved to the controls or another container entirely, restoration reports success immediately — without requesting focus, and with focus nowhere near where it claims. Confirmation by identity was the whole point of the design, and a stale identity quietly turned it back into confirmation by assumption. Every caller now reports both edges, clearing on loss only when that card is still the one on record: a gain elsewhere lands before the matching loss arrives, so an unconditional clear would erase the position that replaced it. The shared catalog grid and the library grid pass both edges through rather than filtering to gains, which is what makes the loss reachable at all. Found by asking whether a defect just found in Calendar had the same shape here. It did, in four already-shipped surfaces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last surface, and the awkward one: two kinds of thing on screen at once, in two separate focus containers, on a root tab. Root tab means recording is click-only, as on Calendar — browsing a result and re-selecting Search must not look like a return. Two containers mean two index-addressed requesters, because one cannot name a position in both. And the namespaced projection means a library item and the requestable title of the same film stay distinct, so a return lands in the section the viewer actually left. Pending is honoured rather than forced. An earlier version passed treatAbsenceAsFinal, which sent every not-yet-loaded target straight to a near miss — the code contradicting a distinction its own projection and tests were built around. Search still does not page TOWARD a target the way the flat surfaces do, but work already in flight now gets waited on, and a fetch that is genuinely still running is stood down from rather than guessed past. Bounded, because an armed return that never resolves would go on suppressing the ordinary submit handoff long after anyone cared. The request row is refreshed on return instead of being left alone or refetched from scratch. Left alone it goes stale, because creating a request in the detail changes the status these cards show; refetched the ordinary way it blanks first, and a restoration loses the card it was aiming at halfway through. refreshInPlace does neither. The explicit-submit handoff is consumed when a return is recorded rather than deferred until it finishes, which only moved the theft later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A mic beside the search field, opening the system recogniser. On a Shield that listens through the remote's own microphone, which is the hardware people expect to be talking into. The remote's mic BUTTON cannot start this. Android TV binds it to the system assistant before any app sees the key, so an on-screen affordance is the only voice entry point an app can offer — hence a control next to the field rather than a hidden gesture. It is a peer of the field, not an icon inside it: a text field's trailing icon is not focusable, and on a remote a control the D-pad cannot reach may as well not exist. Down joins the same chip rail the field uses so the mic is never a dead end. It is hidden outright where no recogniser is installed, rather than offered and inert. Recording is the recogniser's job, not Silo's. Handing off means no RECORD_AUDIO permission and no audio path in this app at all — it can ask for a transcription and nothing else. The manifest needs a <queries> entry to see the recogniser under Android 11 package visibility; without it the availability check comes back empty and the mic silently never appears. A spoken query is a submitted query: same path as typing one, focus handed to the results afterwards, because nobody dictates a title in order to be left sitting on the search field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Also pins the filter chips' UP to the search field rather than leaving it to geometry. The field used to be the nearest focusable thing above that rail and is not any more — the mic now sits to its left, directly over the first chip — so spatial search would send UP to the mic instead. The chips belong to the field wherever it happens to be drawn, and this file already distrusts spatial search for exactly this class of reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…comment Three separate review findings that happened to land in overlapping files. Recorded together rather than split apart after the fact, so the history matches what was actually run and tested as one state. SEARCH — an infinite refresh loop. refreshInPlace() was called from inside an effect keyed on requestSearchSettled, which that very call flips: each refresh relaunched the effect, which refreshed again, whenever resolution did not finish on the first pass. The 10s budget bounded one STUCK fetch and nothing else, so a run of quick successful ones re-armed it indefinitely and returnPending stayed set — permanently suppressing the ordinary submit focus handoff. It now refreshes once per return, and the stand-down budget is absolute per return rather than restarted by every relaunch. Refreshing before resolving also did not do what its comment claimed. The comment said a request target would resolve Pending and wait; it would not, because refreshInPlace PRESERVES results, so the stale card is still present and matches Exact. Pending only happens when the target is ABSENT. The card would take focus and disarm before the response landed, and if that response dropped it, focus went with it. A request-section target now genuinely waits for the refreshed row while a catalog target carries on immediately. And an error state raced: with preserved results plus an error, restoration fired refreshInPlace while the query-keyed effect ran a full search 300ms later, cancelling it and blanking the row. Error recovery now belongs to that effect alone. VOICE — the mic may not have been reachable at all. Compose Foundation's text field consumes D-pad Left as character navigation even when the cursor cannot move further, so Field → Left → Mic is not a route that can be relied on, and pinning every filter chip's Up to the field had closed the only other way in. The first chip now goes up to the mic directly above it, and the mic's Right leads back to the field, giving an entry path that never crosses the field. It still needs proving with a real remote. Availability used queryIntentActivities with no flags, which also counts handlers lacking CATEGORY_DEFAULT — activities startActivityForResult will not launch — so the mic could appear for a recogniser that cannot start. resolveActivity applies the same rule the launch does. The launch itself was wrapped in a blanket runCatching that swallowed every failure, leaving a visible mic that does nothing when pressed and nothing in the log; it now catches ActivityNotFoundException specifically, logs it, and says so on screen. Spoken text bypassed the query length cap that typing obeys, having never gone through the field. EXTRA_MAX_RESULTS is dropped, since only the first result was ever used. EXTRA_LANGUAGE stays unset on purpose: unset follows the device's own speech locale, which is what a household configured, where pinning the app's UI locale would make English work and break a family that speaks Dutch. RESTORATION — a comment still described the replacement timeout as proceeding against whatever list existed. That is the behaviour that WAS the defect; it abandons now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found on a Google TV Streamer: two activities claim the speech intent there — the TV search app and the text-to-speech package — and with no default set the implicit launch becomes a disambiguation dialog. Asking someone to pick an app with a remote before they can say a film title is not voice search. The intent now names a package. Which package is the interesting part, and the device contradicted the obvious answer: the configured VOICE_RECOGNITION_SERVICE on that Streamer points at the text-to-speech package, because that setting names a service for programmatic recognition rather than the best activity to put in front of someone. The voice-interaction/assistant package is the system's designated spoken front end, and on a TV it is the one whose activity is built for a remote microphone — so it is consulted first, and the recognition service only after it. When nothing matches, the intent is left implicit and the system shows its chooser. Worse, but honest: better than silently picking whichever handler happened to be listed first, which on this device would have been the wrong one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
|
The previous attempt did not work on a device, and made one thing worse. It routed chip zero's Up to the mic, to give the button an entry that avoided the text field. But the shell claims DirectionUp in its own preview handler above that row, so the chip's property never decided anything — and when the move the shell performs fails, it hands focus to the top menu. Chip zero's Up therefore left the screen entirely, where before it had reached the search field. Every chip goes back to the field. Left from the field reaches the mic now, taken in the PREVIEW phase. That is the part that matters: Compose's text field consumes Left as cursor movement even when the caret cannot move, so a plain key handler and a focusProperties destination both lose the race — the key never becomes a focus move at all. Previewing it is the same mechanism the shell uses to claim Up, and it beats the field to the event. The cost is that Left no longer walks the caret. On a TV that is a fair trade: text arrives through the on-screen keyboard, which carries its own cursor keys, whereas the mic has no other way in. Reported from a Google TV Streamer: mic visible, nothing reached it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b2e3c6e to
9c9c869
Compare
Reaching anything beside the search field was impossible, and the reason was the keyboard rather than the focus wiring. Raising the IME on focus hands the whole D-pad to a separate window, so no key ever reaches this app and the mic to the left of the field may as well not exist. Two earlier attempts to route around it failed for that reason: nothing an app does wins a race against a window that already has the event. Two changes make it work, and the first one alone does not. The field is read-only until Select. That is what actually keeps the keyboard down — withholding our own show() call achieved nothing, because Compose raises the IME itself whenever an EDITABLE field takes focus. A read-only field can hold focus without one, so the D-pad stays with the screen. Left is still taken in the preview phase. With the keyboard down the text field consumes Left as caret movement anyway, read-only and with nowhere for the caret to go, so the key never becomes a focus move. Only while the keyboard is closed: once it is open the IME owns the D-pad, and Left genuinely should walk the caret then. Back now closes the keyboard instead of falling through to the shell, which popped Search entirely — so the only way to put the keyboard away had been the way out of the screen. Typing costs one Select first. That is the trade, and it is the one the Wholphin client makes; jellyfin-androidtv auto-shows and its own source comments that it would rather not. Verified on a Google TV Streamer: enter Search with the keyboard down, Left focuses the mic, Select raises the keyboard, Back lowers it and stays. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
shared/src/commonTest/kotlin/org/siloserver/silo/viewmodel/PersonalListViewModelGenerationTest.kt (1)
122-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
runTest’s scheduler for the queued coroutine.
PersonalListViewModellaunches work withviewModelScope, so creating a separateTestCoroutineSchedulerhere is unnecessary and adds a second virtual clock. UseStandardTestDispatcher(testScheduler)and advancetestSchedulerin this test.🤖 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 `@shared/src/commonTest/kotlin/org/siloserver/silo/viewmodel/PersonalListViewModelGenerationTest.kt` around lines 122 - 145, Update aRefreshQueuedButNotYetRunStillBlocksPaging to use runTest’s provided testScheduler: construct StandardTestDispatcher(testScheduler) and replace all scheduler.advanceUntilIdle() calls with testScheduler.advanceUntilIdle(). Remove the locally created TestCoroutineScheduler so viewModelScope and the test share one virtual clock.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/calendar/TvCalendarScreen.kt (1)
1146-1153: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider retrying the shelf focus request before reporting the token consumed.
DayShelfcallstargetCardFocusRequester.requestFocus()exactly once, then callsonFocusApplied().onFocusApplied()clearsshelfFocusDay,shelfFocusRequest, andshelfFocusItemIndex, so the requester moves back to card zero and the token cannot be replayed. If the target card is not yet attached when the request runs, the restoration at lines 271-286 waits outTvFocusAcquisitionBudgetMillisand then falls back to the shell handoff.The flat surfaces use
requestFocusUntilObservedwith bounded retries for this reason. A small retry loop here would make the calendar path consistent.♻️ Suggested change
LaunchedEffect(focusRequest) { if (focusRequest > 0 && items.isNotEmpty()) { rowState.scrollToItem(targetCardIndex) - runCatching { targetCardFocusRequester.requestFocus() } + var claimed = runCatching { targetCardFocusRequester.requestFocus() }.getOrDefault(false) + repeat(4) { + if (claimed) return@repeat + androidx.compose.runtime.withFrameNanos { } + claimed = runCatching { targetCardFocusRequester.requestFocus() }.getOrDefault(false) + } onFocusApplied() } }🤖 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/screens/calendar/TvCalendarScreen.kt` around lines 1146 - 1153, Update the DayShelf focus-restoration flow around targetCardFocusRequester.requestFocus() so it retries the request with a bounded retry loop while the target card is not yet attached, matching the existing requestFocusUntilObserved behavior used by flat surfaces. Only call onFocusApplied() after a successful focus acquisition or after the established retry budget is exhausted, preserving the fallback handoff behavior.androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/browse/CatalogLetterIndexViewModelTest.kt (1)
191-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSupply descriptive messages to
awaitStatecalls.
kotlin.test.assertTrueis already imported, so this concern does not affect compilation. Thedescriptionparameter falls back to"expected state"at every call site; passing a short state name helps the failure message identify which state was never reached.🤖 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 `@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/browse/CatalogLetterIndexViewModelTest.kt` around lines 191 - 201, Update every call site of awaitState in CatalogLetterIndexViewModelTest to pass a concise, descriptive state name via its description argument, so assertion failures identify the specific state that was not reached; keep the existing predicate behavior unchanged.
🤖 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/build.gradle.kts`:
- Around line 246-247: Update the release variant configuration in
beforeVariants to replace the deprecated variant.enableUnitTest assignment with
the AGP host-tests API, casting the variant to HasHostTestsBuilder and disabling
HostTestBuilder.UNIT_TEST_TYPE via its hostTests entry.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvSkylineSectionFeed.kt`:
- Around line 418-425: Thread hydration completeness through
TvSkylineSectionFeed by adding a sectionsFullyResolved parameter, defaulting to
true for existing callers, and passing hydration's fullyResolved value from
every call site. Update the remember block's resolveTvReturnTarget invocation to
pass sectionsFullyResolved as sectionsComplete, preserving Pending resolution
while rows are still hydrating.
- Around line 157-162: Update the returnTarget state in TvSkylineSectionFeed to
use saveable state isolated by the owning feed surface, rather than the shared
positional TvReturnTargetSaver slot. Key the saver or composition state with the
feed surface identity so switching between surfaces resets or restores only that
surface’s target, while preserving detailReturnPending behavior.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/library/TvLibraryDetailScreen.kt`:
- Around line 569-580: Update the DisposableEffect keyed by item.contentId in
the restoredItemIndex branch to clear the shared attachment only when its owner
still matches item.contentId. Capture or track the effect’s owner inside the
itemsIndexed scope, and guard onDispose with attachedItemId == item.contentId so
a newer requester attachment is not cleared.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/DevicePairingViewModel.kt`:
- Around line 92-102: Scope all lookup-driven UI state updates in
DevicePairingViewModel to the currently active lookup: increment
lookupGeneration in onCodeChanged to retire pending results, ensure retired
lookups cannot clear isLoading, and clear loading when a decision explicitly
retires the lookup. Preserve only the active lookup’s ability to apply loading,
success, or error state, and add ordering tests covering a late lookup after
onCodeChanged and lookup A completing while lookup B remains pending.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/PersonalListViewModels.kt`:
- Around line 96-115: Update the refresh success path in refresh() to set
hasLoadedOnce when fetched items are published, matching the existing behavior
in load(). Ensure every refresh that publishes content marks the view model as
loaded, including refreshes that supersede the initial load or a prior retry.
---
Nitpick comments:
In
`@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/browse/CatalogLetterIndexViewModelTest.kt`:
- Around line 191-201: Update every call site of awaitState in
CatalogLetterIndexViewModelTest to pass a concise, descriptive state name via
its description argument, so assertion failures identify the specific state that
was not reached; keep the existing predicate behavior unchanged.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/calendar/TvCalendarScreen.kt`:
- Around line 1146-1153: Update the DayShelf focus-restoration flow around
targetCardFocusRequester.requestFocus() so it retries the request with a bounded
retry loop while the target card is not yet attached, matching the existing
requestFocusUntilObserved behavior used by flat surfaces. Only call
onFocusApplied() after a successful focus acquisition or after the established
retry budget is exhausted, preserving the fallback handoff behavior.
In
`@shared/src/commonTest/kotlin/org/siloserver/silo/viewmodel/PersonalListViewModelGenerationTest.kt`:
- Around line 122-145: Update aRefreshQueuedButNotYetRunStillBlocksPaging to use
runTest’s provided testScheduler: construct
StandardTestDispatcher(testScheduler) and replace all
scheduler.advanceUntilIdle() calls with testScheduler.advanceUntilIdle(). Remove
the locally created TestCoroutineScheduler so viewModelScope and the test share
one virtual clock.
🪄 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: cacb2dc5-3d6a-4b3a-a45a-4623d7dac149
📒 Files selected for processing (27)
androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/browse/CatalogLetterIndexViewModelTest.ktandroidTvApp/build.gradle.ktsandroidTvApp/src/androidMain/AndroidManifest.xmlandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvCatalogGrid.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvSkylineSectionFeed.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvFlatReturnRestoration.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvReturnAdapters.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvReturnTarget.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/calendar/TvCalendarScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/collections/TvCollectionDetailScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/library/TvLibraryDetailScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/people/TvPersonDetailScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/personal/TvPersonalScreens.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/requests/TvMyRequestsScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/search/TvSearchReturnProjection.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/search/TvSearchScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/search/TvVoiceSearch.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvReturnAdaptersTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvReturnTargetTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/library/TvLibraryFocusRestoreTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/search/TvSearchReturnProjectionTest.ktshared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/DevicePairingViewModel.ktshared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/PersonalListViewModels.ktshared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/RequestsViewModels.ktshared/src/commonTest/kotlin/org/siloserver/silo/viewmodel/DevicePairingDecisionOrderingTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/viewmodel/PersonalListViewModelGenerationTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.kt
…ings Six findings from review of Silo-Server#168. The first is the significant one and it is a contract this branch wrote and then failed to honour at the call site. HOME RESTORATION — TvReturnAdapters documents that the caller must supply hydration's fullyResolved as sectionsComplete. The Skyline feed passed neither: it took the default of true, and it projects rows that are already filtered to sections WITH items, so an unhydrated placeholder is dropped before the adapter could mark it incomplete. TvReturnResolution.Pending was therefore unreachable on Home and its three guards never ran. While hydration was still filling rows, an absent launch row read as gone rather than late, resolution settled on the nearest survivor, and focus was driven to a card the viewer never opened — retiring the real target on the way. HomeViewModel now publishes sectionsFullyResolved and the feed passes it through. DEVICE PAIRING — the earlier generation guard was incomplete. Changing the code did not retire an in-flight lookup, so a late answer for the previous code could repopulate the details after the viewer typed a different one, showing them a device that is not the one being asked about. isLoading is also owned now: a retired lookup no longer clears a newer lookup's spinner, and a decision releases the flag its retired lookup will never clear. PERSONAL LISTS — hasLoadedOnce was set only in load()'s own branches, so a refresh that overtook the initial load left it false forever. Screens gate their resume re-fetch on that flag, which disabled resume refresh for the life of the view model. A refresh that publishes content now sets it. LIBRARY GRID — attachment disposal was not owner-guarded, the same guard already applied to the catalog grid and My Requests. When the requester moved, the old card's disposal cleared the new card's live attachment and the restoration reported NotReady against a requester that was in fact bound. PROFILE SCREEN — the onboarding step indicator was drawn unconditionally, so switching profiles from the menu showed a "step 3 of 3" progress row to someone who was not onboarding, and its badge overlapped the Manage button. The utility pills also had fixed widths that clipped their labels: "Sign Out" rendered as "Sign". They size to their content now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionScreen.kt (2)
249-258: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStop retries after every focus-owner change.
targetStatereturnsDisposedonly for a different non-nullfocusedProfileId.ProfileTileGridreportsnullfor Add Profile and when no profile tile owns focus. If the viewer moves to Add Profile while retries run, the state remainsReady, so the old target can be requested again and steal focus. Track a focus-change generation or distinguish Add Profile from no focus.Also applies to: 378-390
🤖 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/screens/profiles/TvProfileSelectionScreen.kt` around lines 249 - 258, Update the retry cancellation logic in the targetState block and its corresponding logic near ProfileTileGrid so every focus-owner change, including transitions to Add Profile and no profile focus, invalidates the current retry sequence. Track a focus-change generation or otherwise distinguish Add Profile from no focus, and return TvFocusTargetState.Disposed whenever the current focus owner differs from the retry’s original owner.
229-235: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrack list materialization separately from successful focus anchoring.
tvProfileFocusTargetuseshasMaterializedto identify the first list arrival. This code passeshasAnchored, which remainsfalsewhen the first request is cancelled because the viewer moves focus. On the next profile-list change, the helper returnscurrentIds.first()and can override the viewer's current profile. Store a separate materialization flag.Also applies to: 261-265
🤖 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/screens/profiles/TvProfileSelectionScreen.kt` around lines 229 - 235, Update the LaunchedEffect(profileIds) flow and the corresponding call near the second focus-handling block to pass a dedicated list-materialization flag to tvProfileFocusTarget instead of hasAnchored. Set that flag when the profile list is first materialized, independently of whether focus anchoring succeeds or the request is cancelled, while preserving hasAnchored for tracking successful focus placement.
🤖 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 `@shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/HomeViewModel.kt`:
- Around line 169-178: Serialize all fetchSections() publishers, including
loadSections() and refreshFromRealtime(), through one shared fetch job or
request-sequence mechanism. Before applying results to _uiState in the update
block, reject any result from an older request so a stale partial response
cannot overwrite a newer complete response or regress sectionsFullyResolved.
---
Outside diff comments:
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionScreen.kt`:
- Around line 249-258: Update the retry cancellation logic in the targetState
block and its corresponding logic near ProfileTileGrid so every focus-owner
change, including transitions to Add Profile and no profile focus, invalidates
the current retry sequence. Track a focus-change generation or otherwise
distinguish Add Profile from no focus, and return TvFocusTargetState.Disposed
whenever the current focus owner differs from the retry’s original owner.
- Around line 229-235: Update the LaunchedEffect(profileIds) flow and the
corresponding call near the second focus-handling block to pass a dedicated
list-materialization flag to tvProfileFocusTarget instead of hasAnchored. Set
that flag when the profile list is first materialized, independently of whether
focus anchoring succeeds or the request is cancelled, while preserving
hasAnchored for tracking successful focus placement.
🪄 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: 60769dd0-f8f9-43e9-af1f-8cb720a42798
📒 Files selected for processing (7)
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvSkylineSectionFeed.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/home/TvHomeScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/library/TvLibraryDetailScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionScreen.ktshared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/DevicePairingViewModel.ktshared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/HomeViewModel.ktshared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/PersonalListViewModels.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/PersonalListViewModels.kt
- shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/DevicePairingViewModel.kt
- androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/library/TvLibraryDetailScreen.kt
- androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvSkylineSectionFeed.kt
0320a67 to
ec0c3ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.kt (1)
464-478: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity And Privacy (CWE-74): Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection')
Reachability: External · Exploitability: Moderate
Percent-encode
contentIdin TV playback route builders.
contentIdcomes fromuri.pathSegments.lastOrNull()for launcher deep links.TvRoute.Player,ItemDetail, andAudiobookPlayerinterpolatecontentIddirectly, so values containing?or&can inject query parameters. Percent-encode eachcontentIdpath segment before appending it.🤖 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/navigation/TvAppNavigation.kt` around lines 464 - 478, Update the TV playback route builders used by navigateToTvPlayback, including TvRoute.Player, ItemDetail, and AudiobookPlayer, to percent-encode contentId as a path segment before interpolation. Ensure contentId values containing ? or & remain part of the path and cannot inject query parameters, while preserving existing route behavior for ordinary IDs.
🧹 Nitpick comments (10)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/MountedAudioMatching.kt (1)
99-111: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFold
eac3_jocinto theeac3family.
normalizedAudioTokenstrips the underscore, so a catalog codeceac3_jocbecomeseac3joc. No branch matches it, and theelsereturnseac3joc. A mounted Media3 track reportsaudio/eac3, which canonicalises toeac3. The codec filter inmatchMountedAudioTrackthen empties the pool and returns null, so an Atmos E-AC-3 track always falls back to a server replan instead of switching locally.PlayerViewModel.toAudioMimeTypein this PR already treatseac3_jocas E-AC-3, so the catalog does use that spelling.♻️ Proposed fix
- token.startsWith("ec3") || token == "eac3" || token == "ddp" -> "eac3" + token.startsWith("ec3") || token.startsWith("eac3") || token == "ddp" -> "eac3"🤖 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 `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/MountedAudioMatching.kt` around lines 99 - 111, Update normalizedAudioToken’s E-AC-3 branch to also match the normalized eac3joc token, returning the canonical eac3 value so it aligns with mounted Media3 tracks and the existing PlayerViewModel.toAudioMimeType handling.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt (2)
2656-2668: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftTV has no bounded fallback for a local audio switch that never takes.
The phone implementation bounds this branch with
MAX_LOCAL_AUDIO_ATTEMPTSand then callsreplanForDesiredAudio, becauseAudioTrackManager.selectAudioTrackreturnsUnit, does nothing when the group has gone, and produces no callback. The TVApplybranch reissuesLocalAudioSelectionon every track snapshot with no attempt ceiling and no server fallback. If the mounted group cannot accept the override, the row stays unconfirmed for the whole session and the server is never asked to materialise the track.Consider mirroring the phone bound and falling back to
subtitleTransactions.selectAudio(desired.catalogOrdinal)after a small number of attempts.🤖 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/screens/player/TvPlayerViewModel.kt` around lines 2656 - 2668, The TV AudioReconcileAction.Apply branch can retry indefinitely without recovering when the local track switch is ignored. Mirror the phone implementation’s MAX_LOCAL_AUDIO_ATTEMPTS bound using localAudioAttempt, and once the limit is reached call replanForDesiredAudio or subtitleTransactions.selectAudio(desired.catalogOrdinal) according to the existing TV fallback flow instead of creating another LocalAudioSelection.
2510-2532: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStacked KDoc blocks are attached to the wrong declarations. Each site has two or three consecutive KDoc blocks before one declaration. Kotlin binds only the last block, so the earlier blocks document a member they do not belong to, and IDE and KDoc output show the wrong text.
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt#L2510-L2532: move the "Bounded recovery has given up" block ontoonPlaybackRecoveryExhaustedand the "The screen has shown ... subtitleFailureMessage" block ontoonSubtitleFailureShown; keep only the audio-commit block ononAudioSelectionCommitted.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt#L959-L972: delete the first, superseded KDoc block sodesiredAudioOrdinalcarries one description.androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt#L187-L206: the first block describes the server ordinal helper, notmobileAudioTrackPersistenceUpdate; move it ontoselectedServerAudioTrackIndexor remove it.🤖 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/screens/player/TvPlayerViewModel.kt` around lines 2510 - 2532, Fix the stacked KDoc attachments: in androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt:2510-2532, move the bounded-recovery block to onPlaybackRecoveryExhausted and the subtitleFailureMessage block to onSubtitleFailureShown, leaving only the audio-commit block on onAudioSelectionCommitted. In androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt:959-972, remove the superseded first KDoc so desiredAudioOrdinal has one description. In androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt:187-206, move the server-ordinal KDoc to selectedServerAudioTrackIndex or remove it, rather than leaving it attached to mobileAudioTrackPersistenceUpdate.shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt (1)
99-121: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake
setProfileIdentity()overlay-safe or document the expected behavior.The default call to
setProfileId()thensetProfileToken()can merge the commit into an active temporary scope, while the production implementations refuse the write outright. Document that overlay-safe implementations must override this, or apply the same refusal in the default.🤖 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 `@shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt` around lines 99 - 121, Update setProfileIdentity in TokenManager to document that implementations using temporary overlays must override it with overlay-safe atomic behavior, matching production implementations that reject writes in an active overlay. Alternatively, make the default implementation reject commits when an overlay is active; preserve the existing two-step fallback only for managers without overlay scopes.android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/MountedAudioReorderTest.kt (1)
32-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse camelCase test function names.
The new test functions use backtick prose names. Rename them to camelCase.
android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/MountedAudioReorderTest.kt#L32-L55: Rename the three test functions to camelCase.androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatchTest.kt#L20-L83: Rename the test functions to camelCase.androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvItemDetailNavigationTest.kt#L22-L252: Rename the test functions to camelCase.androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationAttributionTest.kt#L15-L109: Rename the test functions to camelCase.As per coding guidelines,
**/*.{kt,kts}requirescamelCasefor functions and properties.🤖 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 `@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/MountedAudioReorderTest.kt` around lines 32 - 55, Rename all three test functions in android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/MountedAudioReorderTest.kt:32-55 to camelCase. Apply the same camelCase renaming to every test function in androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatchTest.kt:20-83, androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvItemDetailNavigationTest.kt:22-252, and androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationAttributionTest.kt:15-109, without changing test behavior.Source: Coding guidelines
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailScreen.kt (1)
541-555: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe key-cancel guard covers the More Like This restore only.
The handler clears
pendingSimilarContentId, so a directional press cancels the More Like This restore. The cast restore has no equivalent cancel. The loop at lines 298-306 runs up to 40 attempts with two frame waits each, so it keeps callingcastReturnFocus.requestFocus()for roughly 80 frames. If the viewer presses a direction key during that window, the loop continues to fight them for focus.The comment at lines 238-239 records this exact symptom for the cast rail: focus was "held hostage on that card for ~a second after returning". The rationale at lines 541-545 applies equally to both rails.
Clearing
pendingCastFocusIndexalone does not stop the loop, because the loop readscastRestoreFocusedandcastReturnFocusrather than the index. The cast branch needs an ownership check like thestillOwned()pattern the More Like This branch uses at lines 342-344 and 376.♻️ Sketch of the change
.onPreviewKeyEvent { event -> if ( - pendingSimilarContentId != null && event.type == KeyEventType.KeyDown && event.key in tvDirectionalKeys ) { pendingSimilarContentId = null + pendingCastFocusIndex = -1 } false }And in the cast loop, re-check ownership each attempt so the cancel takes effect:
var restored = false for (attempt in 0 until 40) { if (castRestoreFocused.value) { restored = true break } + if (pendingCastFocusIndex < 0) return@LaunchedEffect runCatching { castReturnFocus.requestFocus() } withFrameNanos { } withFrameNanos { } }The early return skips the Play fallback, which is correct: the viewer has already moved focus themselves.
🤖 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/screens/detail/TvItemDetailScreen.kt` around lines 541 - 555, Extend the onPreviewKeyEvent directional-key cancellation to clear the cast restore state as well as pendingSimilarContentId. In the cast restore loop using castRestoreFocused and castReturnFocus, re-check ownership on every attempt with the existing stillOwned() pattern and exit immediately when ownership is lost, bypassing the Play fallback after user-driven focus movement.android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractor.kt (3)
232-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider surfacing
malformedSets.
malformedSetsis incremented at Lines 159, 235, and 245, but nothing reads it. The value never reaches a log line or a diagnostics field, so the drop count is invisible after playback. Each drop does emit its ownSubDiag.logline, so the information is not lost entirely, but a running total would show how many captions a file lost.Include the counter in the existing drop log, or expose it alongside
emittedSets.♻️ Proposed change
} catch (e: Exception) { malformedSets++ org.siloserver.silo.common.player.SubDiag.log( - "SUP set $emittedSets rejected: ${e::class.simpleName}: ${e.message}", + "SUP set $emittedSets rejected (dropped=$malformedSets): " + + "${e::class.simpleName}: ${e.message}", )🤖 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 `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractor.kt` around lines 232 - 239, Surface the accumulated malformedSets count from the PGS extraction flow: update the existing rejection diagnostics around decodeDisplaySet and the other malformed-set paths to include the running total, or expose it alongside emittedSets in the existing diagnostics output. Ensure every increment site contributes to the reported count.
240-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind the unused
OutOfMemoryErrorto_to satisfy detekt.detekt reports
SwallowedExceptionat Line 240. TheExceptionbranch above logse::class.simpleNameande.message; this branch bindseand never reads it. Rename the binding to_, which matches thecatch (_: EOFException)style already used in this file.The decision to catch
OutOfMemoryErrorhere is sound. The block now contains only parsing and cue encoding, both sized by the display set's own declared dimensions, and the sample-queue write moved topublishDisplaySet.♻️ Proposed change
- } catch (e: OutOfMemoryError) { + } catch (_: OutOfMemoryError) {🤖 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 `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractor.kt` around lines 240 - 250, In the OutOfMemoryError catch branch surrounding the malformed-set handling, rename the unused exception binding from e to _ while preserving the existing malformedSets increment, diagnostic log, and null return.Source: Linters/SAST tools
200-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the shadowed
outputlocal at Line 208.
flushDisplaySettakes anoutput: TrackOutputparameter. Line 208 declares a localval output = trackOutput ?: returnwith the same name. The local shadows the parameter, and the parameter is never read afterwards. Kotlin reportsNAME_SHADOWINGfor this, which fails the build if the module enablesallWarningsAsErrors.The re-read also adds no safety. The single caller at Line 148 already resolved
trackOutputat Line 105, andtrackOutputis assigned only ininit.♻️ Proposed change: drop the parameter and keep one resolution point
- private fun flushDisplaySet(output: TrackOutput) { + private fun flushDisplaySet() { if (displaySet.isEmpty()) return @@ val activeParser = parser ?: return - val output = trackOutput ?: return + val output = trackOutput ?: return if (timeUs == C.TIME_UNSET) returnThen update the call site:
appendSegment(segmentType, segmentLength, payload) - flushDisplaySet(output) + flushDisplaySet() return Extractor.RESULT_CONTINUE🤖 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 `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractor.kt` around lines 200 - 252, Remove the unused output parameter from flushDisplaySet and delete the shadowing trackOutput local inside it. Update its call site to invoke flushDisplaySet without passing output, while preserving the existing trackOutput resolution and subsequent publishDisplaySet usage.android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.kt (1)
985-1003: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive assertion so the test cannot pass vacuously.
The test has no assertion. It relies only on
runTestreporting an uncaught exception. The test therefore also passes ifstopAsyncnever invokesstopSessionat all, which is the opposite failure mode. Record that the stop was attempted, then assert it.♻️ Proposed change
val sessionMgr = object : FakeSessionManager() { + var attempts = 0 override suspend fun stopSession(sessionId: String): ApiResult<Unit> = - throw IllegalStateException("stop failed") + throw IllegalStateException("stop failed").also { attempts++ } } @@ lifecycle.stopAsync(expectedSessionId = "sess-a") // Joins the tracked job. If the failure escaped, runTest reports it. lifecycle.acquireOwnershipEpoch() + assertEquals(1, sessionMgr.attempts, "the stop must have been attempted") + assertEquals(SessionState.Idle, lifecycle.state.value)🤖 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 `@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.kt` around lines 985 - 1003, The test `a failing async stop does not escape as an uncaught exception` must verify that `stopSession` was actually invoked. Track the call in the overridden `stopSession`, then assert the recorded attempt after `lifecycle.stopAsync` completes while retaining the existing uncaught-exception check.
🤖 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
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatch.kt`:
- Around line 37-45: Update the matching logic around
deviceLoginOriginMatchesServer so the entries lookup runs whenever
requiredOrigin is present, before returning Active for a null activeServerUrl.
Preserve the Active result for a matching active server, and resolve a known
persisted entry through SwitchRequired even when no active server is configured;
return UnknownServer for unmatched origins.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt`:
- Around line 393-402: Bind pendingLocalAudioCommit to the content generation or
playback identity when queued by commitLocallyAppliedAudio, and discard it when
ownership changes during resetContent or superseded adoption. Update
drainPendingLocalAudioCommit to apply only matching queued commits and keep
transition.context.audioTrackIndex synchronized, ensuring context is rebuilt
after the pending commit is drained. Add a regression test covering an audio
switch followed by a content reset during commit.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerScreen.kt`:
- Around line 350-366: Update the audio-track mounting flow around
viewModel.onMountedAudioChanged() to retry the unresolved
pendingLocalAudioSelection request whenever mounted tracks become available.
Preserve the existing LaunchedEffect(videoBackend) selection behavior, and
ensure the pending request is retried without requiring another user selection.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt`:
- Around line 3961-3967: Move the conditional setDesiredAudio(0, explicit =
false) call into the loadOwners.runIfOwned(loadOwner) block, so it executes only
when this load still owns the state; preserve the existing published return
behavior and audioTracks check.
- Around line 2975-2990: The unresolved call in PlayerViewModel.commitLocalAudio
must be supported by MobileSubtitleTransactionAdapter. Add
commitLocallyAppliedAudio, updating the reducer’s committed
CommittedSubtitle.audioTrackIndex for locally restored audio while leaving
audioPreferenceSpecified unchanged.
In
`@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.kt`:
- Around line 633-639: Update the `start` method so the `requestCount.value`
assignment occurs inside the existing `synchronized(pending)` block, immediately
after adding the new `Pending` entry and computing `pending.size`. Keep the
continuation registration and request-count behavior unchanged while ensuring
the published count is synchronized with pending-list updates.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitlePolicy.kt`:
- Around line 233-241: The persisted audio-resolution helper must remain
ordinal-based: update tvAudioTrackPersistenceUpdate and its callers so
committedAudioTrackIndex is resolved by the audioTracks list position, not
AudioTrack.index. Ensure callers pass the ordinal selected track value,
preserving stable fingerprint persistence for nonzero positions.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt`:
- Around line 3485-3489: Update the selectedCatalogAudio calculation in the
episode handoff to prefer the confirmed desired audio ordinal used by
startProtocolV3Replan, falling back to playbackPlan.selectedTracks.audioIndex
when no confirmed choice exists. Keep resolving the ordinal through
activeVersion.audioTracks.getOrNull, and leave selectedAudioTrack and
hasExplicitAudioSelection unchanged.
In
`@shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt`:
- Around line 271-290: Update setProfileIdentity so it calls
ensureCacheMatchesRegistryLocked() inside mutex.withLock before reading
activeServerId or writing profile fields. Preserve the existing temporaryScope
guard and subsequent identity persistence logic after the cache has been
reconciled.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt`:
- Around line 137-141: Update the credential-bearing request flow around
profileIdentity, profileId, and profileToken to attach Authorization,
X-Profile-Id, and X-Profile-Token only for HTTPS requests. Reject or omit
credentials for cleartext HTTP, and configure followRedirects so credentials are
not propagated when redirecting from HTTPS to HTTP or to a different origin.
---
Outside diff comments:
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.kt`:
- Around line 464-478: Update the TV playback route builders used by
navigateToTvPlayback, including TvRoute.Player, ItemDetail, and AudiobookPlayer,
to percent-encode contentId as a path segment before interpolation. Ensure
contentId values containing ? or & remain part of the path and cannot inject
query parameters, while preserving existing route behavior for ordinary IDs.
---
Nitpick comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractor.kt`:
- Around line 232-239: Surface the accumulated malformedSets count from the PGS
extraction flow: update the existing rejection diagnostics around
decodeDisplaySet and the other malformed-set paths to include the running total,
or expose it alongside emittedSets in the existing diagnostics output. Ensure
every increment site contributes to the reported count.
- Around line 240-250: In the OutOfMemoryError catch branch surrounding the
malformed-set handling, rename the unused exception binding from e to _ while
preserving the existing malformedSets increment, diagnostic log, and null
return.
- Around line 200-252: Remove the unused output parameter from flushDisplaySet
and delete the shadowing trackOutput local inside it. Update its call site to
invoke flushDisplaySet without passing output, while preserving the existing
trackOutput resolution and subsequent publishDisplaySet usage.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/MountedAudioMatching.kt`:
- Around line 99-111: Update normalizedAudioToken’s E-AC-3 branch to also match
the normalized eac3joc token, returning the canonical eac3 value so it aligns
with mounted Media3 tracks and the existing PlayerViewModel.toAudioMimeType
handling.
In
`@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.kt`:
- Around line 985-1003: The test `a failing async stop does not escape as an
uncaught exception` must verify that `stopSession` was actually invoked. Track
the call in the overridden `stopSession`, then assert the recorded attempt after
`lifecycle.stopAsync` completes while retaining the existing uncaught-exception
check.
In
`@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/MountedAudioReorderTest.kt`:
- Around line 32-55: Rename all three test functions in
android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/MountedAudioReorderTest.kt:32-55
to camelCase. Apply the same camelCase renaming to every test function in
androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatchTest.kt:20-83,
androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvItemDetailNavigationTest.kt:22-252,
and
androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationAttributionTest.kt:15-109,
without changing test behavior.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailScreen.kt`:
- Around line 541-555: Extend the onPreviewKeyEvent directional-key cancellation
to clear the cast restore state as well as pendingSimilarContentId. In the cast
restore loop using castRestoreFocused and castReturnFocus, re-check ownership on
every attempt with the existing stillOwned() pattern and exit immediately when
ownership is lost, bypassing the Play fallback after user-driven focus movement.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt`:
- Around line 2656-2668: The TV AudioReconcileAction.Apply branch can retry
indefinitely without recovering when the local track switch is ignored. Mirror
the phone implementation’s MAX_LOCAL_AUDIO_ATTEMPTS bound using
localAudioAttempt, and once the limit is reached call replanForDesiredAudio or
subtitleTransactions.selectAudio(desired.catalogOrdinal) according to the
existing TV fallback flow instead of creating another LocalAudioSelection.
- Around line 2510-2532: Fix the stacked KDoc attachments: in
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt:2510-2532,
move the bounded-recovery block to onPlaybackRecoveryExhausted and the
subtitleFailureMessage block to onSubtitleFailureShown, leaving only the
audio-commit block on onAudioSelectionCommitted. In
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt:959-972,
remove the superseded first KDoc so desiredAudioOrdinal has one description. In
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt:187-206,
move the server-ordinal KDoc to selectedServerAudioTrackIndex or remove it,
rather than leaving it attached to mobileAudioTrackPersistenceUpdate.
In `@shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt`:
- Around line 99-121: Update setProfileIdentity in TokenManager to document that
implementations using temporary overlays must override it with overlay-safe
atomic behavior, matching production implementations that reject writes in an
active overlay. Alternatively, make the default implementation reject commits
when an overlay is active; preserve the existing two-step fallback only for
managers without overlay scopes.
🪄 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: 37e86add-d990-4486-84cd-828d8e73a38d
📒 Files selected for processing (89)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/MediaAuthSession.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackTeardownGate.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/audio/DelayAudioProcessor.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/backend/VideoPlaybackBackend.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractor.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/SubripPayloadNormalizer.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/AudioReconcile.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/EpisodeSelectionHandoff.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/MountedAudioMatching.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/PlaybackStartupStallDetector.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/PostResumeVideoStallDetector.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/network/WatchTogetherRealtimeWebSocketTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/audio/DelayAudioProcessorTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractorTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/subtitle/SubripPayloadNormalizerTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/AudioReconcileTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/MountedAudioMatchingTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/MountedAudioReorderTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/PostResumeVideoStallDetectorTest.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/PushNotificationPresenter.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/BottomNavBar.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkRoutes.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatch.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRoute.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/MainScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/DevicePairingWrongServerScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleAutoSelection.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlaybackStatsSheet.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/PINEntryDialog.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationAttributionTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkEncodingTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParserTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatchTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRouteTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/performance/MobilePlayerLifecyclePerformanceSourceTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/MobileAudioTrackSelectionTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleAutoSelectionTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapterTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.ktandroidTvApp/build.gradle.ktsandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvSkylineSectionFeed.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAudiobookRouting.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvRoute.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvAudiobookDetailHero.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvPlaybackFormatting.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/home/TvHomeScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/library/TvLibraryDetailScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerHud.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitlePolicy.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvVideoPlaybackStarter.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvItemDetailNavigationTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvPlayerRouteTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvPlaybackFormattingTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/PlayerTrackEntriesTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitleIntegrationPolicyTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleSettlementOwnershipTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleTransactionAdapterTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvWatchTogetherSurfaceSourceTest.ktshared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.ktshared/src/androidUnitTest/kotlin/org/siloserver/silo/network/EncryptedTokenManagerScopeGenerationTest.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/playback/SubtitleTransition.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/profile/ProfileModels.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/PlaybackRealtimeClient.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.ktshared/src/commonMain/kotlin/org/siloserver/silo/repository/ProfileRepository.ktshared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/HomeViewModel.ktshared/src/commonTest/kotlin/org/siloserver/silo/network/SiloAuthPluginPinTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/repository/ProfileIdentityCommitTest.kt
💤 Files with no reviewable changes (1)
- android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/PostResumeVideoStallDetectorTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/home/TvHomeScreen.kt
- androidTvApp/build.gradle.kts
- androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/library/TvLibraryDetailScreen.kt
| if (requiredOrigin == null) return DeviceLoginServerMatch.Active | ||
| if (activeServerUrl == null) return DeviceLoginServerMatch.Active | ||
| if (deviceLoginOriginMatchesServer(requiredOrigin, activeServerUrl)) { | ||
| return DeviceLoginServerMatch.Active | ||
| } | ||
| val known = entries.firstOrNull { deviceLoginOriginMatchesServer(requiredOrigin, it.url) } | ||
| return known | ||
| ?.let(DeviceLoginServerMatch::SwitchRequired) | ||
| ?: DeviceLoginServerMatch.UnknownServer(requiredOrigin) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine whether ServerRegistry can hold entries with no active server.
fd -e kt -g '*ServerRegistry*' | xargs rg -nP -C8 'activeServerId|activeEntry|fun\s+switchTo|fun\s+remove'
# Find callers that detach the active server.
rg -nP -C4 'switchActiveServer\s*\(\s*null\s*\)|switchTo\s*\(\s*null\s*\)' --type=kotlinRepository: Silo-Server/silo-android
Length of output: 31496
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Candidate files"
fd -e kt DeviceLoginServerMatch.kt ServerRegistry.kt AndroidServerRegistry.kt
echo
echo "## DeviceLoginServerMatch.kt"
file="$(fd -e kt DeviceLoginServerMatch.kt | head -n1)"
if [ -n "${file:-}" ]; then
cat -n "$file"
fi
echo
echo "## ServerRegistry.kt"
sr="$(fd -e kt '^ServerRegistry\.kt$' | head -n1)"
if [ -n "${sr:-}" ]; then
cat -n "$sr"
fi
echo
echo "## AndroidServerRegistry.kt key sections"
ar="$(fd -e kt '^AndroidServerRegistry\.kt$' | head -n1)"
if [ -n "${ar:-}" ]; then
sed -n '1,260p' "$ar" | cat -n
sed -n '260,320p' "$ar" | cat -n
fi
echo
echo "## Callers of registry APIs used from device login/pairing context"
rg -n -C3 'deviceLoginServerMatch|activeEntry|activeServerId|entries|TokenManager\.switchActiveServer|signOutCurrentServer|remove\(|switchTo\s*\(' --type=kotlin androidApp shared || trueRepository: Silo-Server/silo-android
Length of output: 357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Candidate files"
fd -e kt 'DeviceLoginServerMatch.kt|ServerRegistry.kt|AndroidServerRegistry.kt'
echo
echo "## DeviceLoginServerMatch.kt"
while IFS= read -r f; do
case "$f" in */DeviceLoginServerMatch.kt) echo "### $f"; cat -n "$f"; esac
done < <(fd -e kt 'DeviceLoginServerMatch.kt')
echo
echo "## ServerRegistry.kt"
while IFS= read -r f; do
case "$f" in */ServerRegistry.kt) echo "### $f"; cat -n "$f"; esac
done < <(fd -e kt 'ServerRegistry.kt')
echo
echo "## AndroidServerRegistry.kt key sections"
while IFS= read -r f; do
case "$f" in */AndroidServerRegistry.kt)
echo "### $f"
sed -n '1,260p' "$f" | cat -n
sed -n '260,320p' "$f" | cat -n
esac
done < <(fd -e kt 'AndroidServerRegistry.kt')
echo
echo "## Callers of registry APIs used from device login/pairing context"
rg -n -C3 'deviceLoginServerMatch|activeEntry|activeServerId|entries|TokenManager\.switchActiveServer|signOutCurrentServer|remove\(|switchTo\s*\(' --type=kotlin androidApp shared || trueRepository: Silo-Server/silo-android
Length of output: 422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## DeviceLoginServerMatch.kt"
for f in $(fd -e kt 'DeviceLoginServerMatch.kt'); do
echo "### $f"
cat -n "$f"
done
echo
echo "## ServerRegistry.kt"
f="$(fd -e kt '^ServerRegistry\.kt$' | head -n1)"
if [ -n "${f:-}" ]; then
echo "### $f"
cat -n "$f"
fi
echo
echo "## AndroidServerRegistry.kt key sections"
f="$(fd -e kt '^AndroidServerRegistry\.kt$' | head -n1)"
if [ -n "${f:-}" ]; then
echo "### $f"
sed -n '1,260p' "$f" | cat -n
sed -n '260,320p' "$f" | cat -n
fi
echo
echo "## Usages of deviceLoginServerMatch and server activation APIs"
rg -n -C3 'deviceLoginServerMatch|activeEntry|activeServerId|activeServerUrl|entries|TokenManager\.switchActiveServer|signOutCurrentServer|remove\(|switchTo\s*\(' --type kotlin shared androidApp || trueRepository: Silo-Server/silo-android
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## DeviceLoginServerMatch.kt"
f="$(fd -e kt '^DeviceLoginServerMatch\.kt$' | head -n1)"
[ -n "$f" ] && cat -n "$f"
echo
echo "## ServerRegistry.kt"
f="$(fd -e kt '^ServerRegistry\.kt$' | head -n1)"
[ -n "$f" ] && cat -n "$f"
echo
echo "## AndroidServerRegistry.kt persistence and active selection"
f="$(fd -e kt '^AndroidServerRegistry\.kt$' | head -n1)"
[ -n "$f" ] && sed -n '180,230p' "$f" | cat -n
echo
echo "## Read-only model check: persisted activeServerId can be null while entries persist"
python3 - <<'PY'
import json, re
for text in [
open('shared/src/commonMain/kotlin/org/siloserver/silo/network/ServerRegistry.kt', encoding='utf-8').read(),
open('shared/src/androidMain/kotlin/org/siloserver/silo/network/AndroidServerRegistry.kt', encoding='utf-8').read(),
]:
print("registry_active_id_nullable=", bool(re.search(r'activeServerId:\s*StateFlow<String\?>', text)))
print("persist_active_id_nullable=", bool(re.search(r'val\s+activeServerId:\s*String\?[^=]', text)))
PY
echo
echo "## Focussed usages around device login/pairing and server detachment"
rg -n -C3 'deviceLoginServerMatch|activeEntry|activeServerId|activeServerUrl|entries|switchActiveServer\s*\(\s*null\s*(?:,\s*[^)]*)?\)|signOutCurrentServer|remove\(|switchTo\s*\(' --type kotlin shared androidAppRepository: Silo-Server/silo-android
Length of output: 50381
Resolve an inactive-but-known server to SwitchRequired.
This branch returns Active while activeServerUrl is null, before checking entries. ServerRegistry allows persisted entries while activeServerId is null, so a pairing link for a known-but-inactive server opens DevicePairingScreen instead of DevicePairingWrongServerScreen. The code lookup then fails with no active server and hides the switch choice. Move the entries lookup ahead of the active-server null check when requiredOrigin is present.
🤖 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
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatch.kt`
around lines 37 - 45, Update the matching logic around
deviceLoginOriginMatchesServer so the entries lookup runs whenever
requiredOrigin is present, before returning Active for a null activeServerUrl.
Preserve the Active result for a matching active server, and resolve a known
persisted entry through SwitchRequired even when no active server is configured;
return UnknownServer for unmatched origins.
| /** Applied once an in-flight commit finishes; see [commitLocallyAppliedAudio]. */ | ||
| private var pendingLocalAudioCommit: Int? = null | ||
|
|
||
| private fun drainPendingLocalAudioCommit() { | ||
| val queued = pendingLocalAudioCommit ?: return | ||
| pendingLocalAudioCommit = null | ||
| transition = transition.copy( | ||
| committed = transition.committed.copy(audioTrackIndex = queued), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bind queued local audio to its playback context.
pendingLocalAudioCommit stores only an ordinal. If resetContent() occurs during a commit, finishSupersededAdoption() can apply an audio ordinal from the old content to the new content.
On successful adoption, context is rebuilt from the old committed ordinal before drainPendingLocalAudioCommit() updates only transition. Later replans can then use stale context.audioTrackIndex.
Store the content generation or playback identity with the queued value. Drop it when ownership changes. Drain it before rebuilding context, or update context in the drain method. Add a regression test for an audio switch followed by content reset during commit.
Also applies to: 851-860, 880-882
🤖 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
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt`
around lines 393 - 402, Bind pendingLocalAudioCommit to the content generation
or playback identity when queued by commitLocallyAppliedAudio, and discard it
when ownership changes during resetContent or superseded adoption. Update
drainPendingLocalAudioCommit to apply only matching queued commits and keep
transition.context.audioTrackIndex synchronized, ensuring context is rebuilt
after the pending commit is drained. Add a regression test covering an audio
switch followed by a content reset during commit.
| // Applies a local audio switch. The ViewModel does not commit on this call: | ||
| // AudioTrackManager returns Unit and does nothing silently when the group is | ||
| // absent, so it waits for a snapshot showing the target selected. | ||
| LaunchedEffect(videoBackend) { | ||
| val backend = videoBackend ?: return@LaunchedEffect | ||
| viewModel.pendingLocalAudioSelection.collect { request -> | ||
| request ?: return@collect | ||
| backend.selectAudioTrack( | ||
| VideoPlayerTrackEntry( | ||
| index = request.targetOrdinal, | ||
| label = "", | ||
| language = null, | ||
| isSelected = true, | ||
| ), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt --items all
rg -n -C 6 'pendingLocalAudioSelection|onMountedAudioChanged|onSelectAudio|selectAudioTrack' \
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player \
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/playerRepository: Silo-Server/silo-android
Length of output: 33614
Retry pre-mount audio selections when mounted tracks become available.
If backend.selectAudioTrack() runs before Media3 publishes an audio group, it does nothing. Make sure viewModel.onMountedAudioChanged() re-tries the unresolved pendingLocalAudioSelection request, otherwise the selection remains pending until the user selects the track again.
🤖 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
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerScreen.kt`
around lines 350 - 366, Update the audio-track mounting flow around
viewModel.onMountedAudioChanged() to retry the unresolved
pendingLocalAudioSelection request whenever mounted tracks become available.
Preserve the existing LaunchedEffect(videoBackend) selection behavior, and
ensure the pending request is retried without requiring another user selection.
| private fun commitLocalAudio(desired: DesiredAudio) { | ||
| _uiState.update { it.copy(selectedAudioIndex = desired.catalogOrdinal) } | ||
| val state = _uiState.value | ||
| routeIntentState.applyCommittedTracks( | ||
| contentId = state.contentId, | ||
| committedAudioServerIndex = desired.catalogOrdinal, | ||
| committedSubtitleIdentity = state.committedSubtitleIdentity, | ||
| transactionFailed = false, | ||
| transactionActive = mobileSubtitleTransactions.hasActiveTransaction, | ||
| ) | ||
| // The reducer's committed audio is what the next subtitle transaction | ||
| // stages and what teardown persists, so it has to move too -- updating | ||
| // the context alone left it stale and the choice got undone. | ||
| mobileSubtitleTransactions.commitLocallyAppliedAudio(desired.catalogOrdinal) | ||
| if (desired.explicit) persistDesiredAudio(desired.catalogOrdinal) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate commitLocallyAppliedAudio and check how it mutates committed audio state.
set -euo pipefail
fd -i 'MobileSubtitleTransactionAdapter*.kt' | while IFS= read -r f; do
ast-grep outline "$f" --items all
done
echo '--- definition and call sites ---'
rg -n -C 8 'commitLocallyAppliedAudio' --type=kotlinRepository: Silo-Server/silo-android
Length of output: 5241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate symbols and method definitions ---'
rg -n -C 12 'commitLocallyAppl|audioTrackIndex|audioPreferenceSpecified|CommittedSubtitle|PendingLocalRestore|pendingLocalRestore' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player --type=kotlin
echo '--- inspect MobileSubtitleTransactionAdapter around relevant regions ---'
sed -n '1,180p' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt
sed -n '1200,1380p' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt
rg -n 'fun .*resetContent|fun .*restoreCommittedLocalMount|fun .*beginLocalRestore|fun .*reportMountedSelection|fun .*processCandidate|fun .*finishSuccessfullyAdopt|class PendingLocalRestore|data class PendingLocalRestore' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.ktRepository: Silo-Server/silo-android
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- exact definitions/call sites ---'
rg -n 'fun .*commitLocallyAppliedAudio|commitLocallyAppliedAudio\(' --type=kotlin
echo '--- PlayerViewModel around audio commit ---'
sed -n '2930,3020p' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt
echo '--- focused MobileSubtitleTransactionAdapter symbols ---'
rg -n -C 10 'class PendingLocalRestore|data class PendingLocalRestore|restoreCommittedLocalMount|beginLocalRestore|reportMountedSelection|processCandidate|persistCommittedSelection|audioTrackIndex|audioPreferenceSpecified|CommittedSubtitle|pendingLocalRestore' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt
echo '--- targeted lines ---'
rg -n 'private object PendingLocalRestore|data class PendingLocalRestore|internal interface Adopt|enum class AdoptionOutcome|fun .*persist|private mut|^class MobileSubtitleTransactionAdapter|^companion objectMobileSubtitleTransactionAdapter' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.ktRepository: Silo-Server/silo-android
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- target file size ---'
wc -l androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt
echo '--- PlayerViewModel audio commit ---'
sed -n '2930,3020p' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt
echo '--- all Kotlin occurrences of commitLocallyAppliedAudio ---'
rg -n -C 8 'commitLocallyAppliedAudio' androidApp/src/androidMain/kotlin --type=kotlin || true
echo '--- relevant MobileSubtitleTransactionAdapter sections ---'
sed -n '124,180p' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kт
sed -n '200,320p' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt
sed -n '1215,1340p' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt
echo '--- CommittedSubtitle model ---'
rg -n -C 12 'data class CommittedSubtitle|sealed class CommittedSubtitle|CommittedSubtitle' --type=kotlinRepository: Silo-Server/silo-android
Length of output: 5230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- PlayerViewModel commitLocalAudio region ---'
rg -n -C 25 'commitLocalAudio|mobileSubtitleTransactions\.' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt || true
echo '--- relevant MobileSubtitleTransactionAdapter sections ---'
sed -n '124,180p' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt
sed -n '200,320p' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt
sed -n '1215,1340p' androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt
echo '--- all occurrences in repo of commitLocallyAppliedAudio and related adapter methods ---'
rg -n -C 4 'commitLocallyAppliedAudio|restoreCommittedLocalMount|beginLocalRestore|reportMountedSelection|processCandidate|persistCommittedSelection|CommittedSubtitle|audioTrackIndex|audioPreferenceSpecified' . || trueRepository: Silo-Server/silo-android
Length of output: 50381
Fix the unresolved commitLocallyAppliedAudio call.
PlayerViewModel.kt calls mobileSubtitleTransactions.commitLocallyAppliedAudio(desired.catalogOrdinal), but MobileSubtitleTransactionAdapter does not define that method. Add the method and make it update the reducer’s committed CommittedSubtitle.audioTrackIndex for the non-explicit restore path without setting audioPreferenceSpecified.
🤖 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
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt`
around lines 2975 - 2990, The unresolved call in
PlayerViewModel.commitLocalAudio must be supported by
MobileSubtitleTransactionAdapter. Add commitLocallyAppliedAudio, updating the
reducer’s committed CommittedSubtitle.audioTrackIndex for locally restored audio
while leaving audioPreferenceSpecified unchanged.
|
|
||
| // Downloaded playback publishes the catalog and hardcodes ordinal 0, but | ||
| // Media3 still picks its own default from the file's tracks -- so the | ||
| // intent has to exist here too or a multi-audio download cannot be | ||
| // corrected. | ||
| if (_uiState.value.audioTracks.isNotEmpty()) setDesiredAudio(0, explicit = false) | ||
| return published |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Seed the downloaded-playback audio intent only when this load still owns the state.
loadOwners.runIfOwned(loadOwner) returns false when a newer load superseded this one, and every other publication site in this file returns early on that result. This call runs unconditionally after the block. When the load is superseded, setDesiredAudio(0, ...) still executes and reads _uiState.value, which now belongs to the newer load. It then bumps desiredAudioGeneration, clears _pendingLocalAudioSelection, and installs a DesiredAudio whose fileId is the newer load's file with catalogOrdinal = 0. The newer load's audio intent is discarded and the player is driven to ordinal 0.
Move the seed inside the owned block.
🐛 Proposed fix
Log.i(
TAG,
"tryLocalPlayback: serving ${media.displayName} (${media.sizeBytes}B) for content=$contentId (sidecar id=${sidecar.record.id})",
)
+ // Downloaded playback publishes the catalog and hardcodes ordinal 0, but
+ // Media3 still picks its own default from the file's tracks -- so the
+ // intent has to exist here too or a multi-audio download cannot be
+ // corrected.
+ if (_uiState.value.audioTracks.isNotEmpty()) setDesiredAudio(0, explicit = false)
}
-
- // Downloaded playback publishes the catalog and hardcodes ordinal 0, but
- // Media3 still picks its own default from the file's tracks -- so the
- // intent has to exist here too or a multi-audio download cannot be
- // corrected.
- if (_uiState.value.audioTracks.isNotEmpty()) setDesiredAudio(0, explicit = false)
return published
}🤖 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
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt`
around lines 3961 - 3967, Move the conditional setDesiredAudio(0, explicit =
false) call into the loadOwners.runIfOwned(loadOwner) block, so it executes only
when this load still owns the state; preserve the existing published return
behavior and audioTracks check.
| override suspend fun start(request: VideoPlaybackStartRequest): VideoPlaybackStartResult = | ||
| suspendCoroutine { continuation -> | ||
| synchronized(pending) { | ||
| requestCount.value = synchronized(pending) { | ||
| pending += Pending(request, continuation) | ||
| pending.size | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Publish the request count inside the lock.
The assignment to requestCount.value is outside synchronized(pending). Only the size computation is inside. If two start calls run on different threads, both can compute their size inside the lock and then write outside it in reverse order. requestCount then settles at 1 while two requests are pending, and awaitRequestCount(2) blocks until the 30-second real deadline.
The synchronized(pending) guard shows that concurrent access is expected here, and the load path uses Dispatchers.IO before reaching this starter. Move the publish inside the lock.
🔒️ Proposed fix
override suspend fun start(request: VideoPlaybackStartRequest): VideoPlaybackStartResult =
suspendCoroutine { continuation ->
- requestCount.value = synchronized(pending) {
+ synchronized(pending) {
pending += Pending(request, continuation)
- pending.size
+ requestCount.value = pending.size
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override suspend fun start(request: VideoPlaybackStartRequest): VideoPlaybackStartResult = | |
| suspendCoroutine { continuation -> | |
| synchronized(pending) { | |
| requestCount.value = synchronized(pending) { | |
| pending += Pending(request, continuation) | |
| pending.size | |
| } | |
| } | |
| override suspend fun start(request: VideoPlaybackStartRequest): VideoPlaybackStartResult = | |
| suspendCoroutine { continuation -> | |
| synchronized(pending) { | |
| pending += Pending(request, continuation) | |
| requestCount.value = pending.size | |
| } | |
| } |
🤖 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
`@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.kt`
around lines 633 - 639, Update the `start` method so the `requestCount.value`
assignment occurs inside the existing `synchronized(pending)` block, immediately
after adding the new `Pending` entry and computing `pending.size`. Keep the
continuation registration and request-count behavior unchanged while ensuring
the published count is synchronized with pending-list updates.
| // An ORDINAL into audioTracks: audio carries no index on the wire, so | ||
| // matching on AudioTrack.index found nothing for any ordinal above 0 and | ||
| // silently Preserved — the chosen track was never persisted, so reopening | ||
| // the item lost it. | ||
| committedAudioTrackIndex | ||
| ?.let { selected -> audioTracks.singleOrNull { it.index == selected } } | ||
| ?.let(audioTracks::getOrNull) | ||
| ?.let(::audioTrackFingerprint) | ||
| ?.let(TrackSelectionFingerprintUpdate::Set) | ||
| ?: TrackSelectionFingerprintUpdate.Preserve |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the removed symbol has no references and inspect ordinal call sites.
rg -n -C4 'resolveTvPersistedAudioPlayerOrdinal'
rg -n -C6 'resolveTvRemoteAudioIntent|tvAudioTrackPersistenceUpdate'
rg -n -C3 'committedAudioTrackIndex'Repository: Silo-Server/silo-android
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -u
echo "== locate file =="
fd -a 'TvPlayerSubtitlePolicy\.kt$' . || true
echo "== git status/diff stat =="
git status --short
git diff --stat || true
echo "== outline =="
file="$(fd 'TvPlayerSubtitlePolicy\.kt$' . | head -n1)"
if [ -n "$file" ]; then
ast-grep outline "$file" || true
echo "== relevant lines 200-290 =="
sed -n '200,290p' "$file" | nl -ba -v200
fi
echo "== search symbols across repo =="
rg -n -C3 'resolveTvPersistedAudioPlayerOrdinal|resolveTvRemoteAudioIntent|tvAudioTrackPersistenceUpdate|committedAudioTrackIndex|AudioTrack\.index|audioTracks\.getOrNull|audioTracks\.indices' .Repository: Silo-Server/silo-android
Length of output: 25675
🏁 Script executed:
#!/bin/bash
set -u
file="androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitlePolicy.kt"
view="androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitleIntegrationPolicyTest.kt"
vm="androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt"
echo "== TvPlayerSubtitlePolicy implementation 240-280 =="
sed -n '240,280p' "$file"
echo "== remoteSelectAudio and resolveTvPersistedAudioPlayerOrdinal implementation =="
sed -n '2808,2825p' "$vm"
sed -n '3128,3150p' "$vm"
echo "== current diff context for TvPlayerSubtitlePolicy and TvPlayerViewModel =="
git diff -- androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitlePolicy.kt androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitleIntegrationPolicyTest.kt | sed -n '1,240p'
echo "== compile-like call-site analysis =="
python3 - <<'PY'
from pathlib import Path, PurePosixPath
import re
def text(p):
return Path(p).read_text()
def occurrences(name):
pat = re.compile(r'(?<![A-Za-z0-9_])' + re.escape(name) + r'(?![A-Za-z0-9_])')
return [(p, m.span()+text(p).count('\n') for m in pat.finditer(text(p))) for p in ['androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitlePolicy.kt', 'androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt', 'androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitleIntegrationPolicyTest.kt']]
for name in ['resolveTvPersistedAudioPlayerOrdinal', 'resolvedCommittedAudioIndex']:
print(f'{name}:')
for p in ['androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitlePolicy.kt', 'androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt', 'androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitleIntegrationPolicyTest.kt']:
hits = [(text(p).count('\n',0,m.start())+1) for m in re.finditer(name, text(p))]
print(' ', p, hits)
PYRepository: Silo-Server/silo-android
Length of output: 4388
🏁 Script executed:
#!/bin/bash
set -u
tv="androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitlePolicy.kt"
vm="androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt"
echo "== resolved helper definitions =="
sed -n '110,135p' "$tv"
sed -n '168,190p' "$tv"
echo "== TvPlayerViewModel persisted track resolution call site =="
sed -n '3134,3148p' "$vm"
echo "== git diff names =="
git diff --name-only || true
echo "== git diff hunks for TvPlayer files =="
git diff -- "$tv" "$vm" | sed -n '1,260p' || true
echo "== call-site verifier =="
python3 - <<'PY'
from pathlib import Path
import re
files = [
"androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitlePolicy.kt",
"androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt",
"androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitleIntegrationPolicyTest.kt",
]
for name in ["resolveTvPersistedAudioPlayerOrdinal", "resolvedCommittedAudioIndex", "committedAudioTrackIndex", "audioTracks.singleOrNull"]:
print(name)
for f in files:
content = Path(f).read_text()
hits = [content.count('\n', 0, m.start()) + 1 for m in re.finditer(re.escape(name), content)]
print(f" {f}: {hits}")
PYRepository: Silo-Server/silo-android
Length of output: 4172
Keep the persisted audio resolution API ordinal-only.
tvAudioTrackPersistenceUpdate still matches committedAudioTrackIndex against AudioTrack.index, so the existing TvPlayerViewModel persistence path can skip stable matches and not persist the selected track. Update that helper to use an ordinal match, or pass an ordinal value into it; callers still passing the wire-level AudioTrack.index will use the wrong selected track.
🤖 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/screens/player/TvPlayerSubtitlePolicy.kt`
around lines 233 - 241, The persisted audio-resolution helper must remain
ordinal-based: update tvAudioTrackPersistenceUpdate and its callers so
committedAudioTrackIndex is resolved by the audioTracks list position, not
AudioTrack.index. Ensure callers pass the ordinal selected track value,
preserving stable fingerprint persistence for nonzero positions.
| selectedAudioTrack = state.audioTracks.firstOrNull { it.isSelected }, | ||
| // The catalog row the plan selected, by ordinal — audio's contract. | ||
| selectedCatalogAudio = state.playbackPlan?.selectedTracks?.audioIndex | ||
| ?.let { activeVersion?.audioTracks?.getOrNull(it) }, | ||
| hasExplicitAudioSelection = manualAudioSelectionApplied, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The episode handoff can carry the pre-switch audio row after a local switch.
selectedCatalogAudio reads only state.playbackPlan?.selectedTracks?.audioIndex. A local audio switch is the case this PR adds: selectAudioOption finds the track already mounted, returns before any replan, and confirmDesiredAudio marks the choice committed. The plan is never updated, so playbackPlan.selectedTracks.audioIndex still names the previous track.
manualAudioSelectionApplied is raised on that same local confirmation, so hasExplicitAudioSelection is true and captureTvEpisodeSelectionHandoff builds a TRACK intent from the stale catalog row. The next episode then starts on the audio the viewer switched away from.
Prefer the confirmed desired ordinal, the same precedence startProtocolV3Replan already applies at Line 2157.
🐛 Proposed fix
- // The catalog row the plan selected, by ordinal — audio's contract.
- selectedCatalogAudio = state.playbackPlan?.selectedTracks?.audioIndex
- ?.let { activeVersion?.audioTracks?.getOrNull(it) },
+ // The viewer's confirmed ordinal wins; a locally applied switch never
+ // moves the plan, so the plan alone names the previous track.
+ selectedCatalogAudio = (
+ state.desiredAudioOrdinal?.takeIf { state.desiredAudioConfirmed }
+ ?: state.playbackPlan?.selectedTracks?.audioIndex
+ )?.let { activeVersion?.audioTracks?.getOrNull(it) },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| selectedAudioTrack = state.audioTracks.firstOrNull { it.isSelected }, | |
| // The catalog row the plan selected, by ordinal — audio's contract. | |
| selectedCatalogAudio = state.playbackPlan?.selectedTracks?.audioIndex | |
| ?.let { activeVersion?.audioTracks?.getOrNull(it) }, | |
| hasExplicitAudioSelection = manualAudioSelectionApplied, | |
| selectedAudioTrack = state.audioTracks.firstOrNull { it.isSelected }, | |
| // The viewer's confirmed ordinal wins; a locally applied switch never | |
| // moves the plan, so the plan alone names the previous track. | |
| selectedCatalogAudio = ( | |
| state.desiredAudioOrdinal?.takeIf { state.desiredAudioConfirmed } | |
| ?: state.playbackPlan?.selectedTracks?.audioIndex | |
| )?.let { activeVersion?.audioTracks?.getOrNull(it) }, | |
| hasExplicitAudioSelection = manualAudioSelectionApplied, |
🤖 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/screens/player/TvPlayerViewModel.kt`
around lines 3485 - 3489, Update the selectedCatalogAudio calculation in the
episode handoff to prefer the confirmed desired audio ordinal used by
startProtocolV3Replan, falling back to playbackPlan.selectedTracks.audioIndex
when no confirmed choice exists. Keep resolving the ordinal through
activeVersion.audioTracks.getOrNull, and leave selectedAudioTrack and
hasExplicitAudioSelection unchanged.
| override suspend fun setProfileIdentity(profileId: String?, profileToken: String?) { | ||
| mutex.withLock { | ||
| // A temporary overlay owns its own identity for the lifetime of a | ||
| // remote-playback handoff. Merging a profile commit into it is how | ||
| // you get the exact defect this method exists to prevent: writing | ||
| // the new profile id beside the overlay's old token. Leave it | ||
| // alone; the repository rejects the commit outright. | ||
| if (temporaryScope != null) return@withLock | ||
| val serverId = activeServerId ?: return | ||
| if (this.profileId == profileId && this.profileToken == profileToken) return | ||
| this.profileId = profileId | ||
| this.profileToken = profileToken | ||
| val idKey = serverScopedKey(serverId, KEY_PROFILE_ID) | ||
| val tokenKey = serverScopedKey(serverId, KEY_PROFILE_TOKEN) | ||
| prefs.edit().apply { | ||
| if (profileId == null) remove(idKey) else putString(idKey, profileId) | ||
| if (profileToken == null) remove(tokenKey) else putString(tokenKey, profileToken) | ||
| }.apply() | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find selectProfile call sites and whether they pass expectedScope.
rg -nP -C6 '\bselectProfile\s*\(' --type=kotlin
# Confirm which write paths reconcile and which do not.
fd -e kt -g 'EncryptedTokenManagerImpl.kt' | xargs rg -nP -C4 'ensureCacheMatchesRegistryLocked|override suspend fun setProfile|override suspend fun getProfile'
# Find direct registry switches that bypass switchActiveServer.
rg -nP -C4 '\bregistry\.switchTo\s*\(|serverRegistry\.switchTo\s*\(' --type=kotlinRepository: Silo-Server/silo-android
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -u
echo "== repo files =="
git ls-files | sed -n '1,120p'
echo "== locate file =="
fd -a 'EncryptedTokenManagerImpl\.kt' . || true
fd -a 'EncryptedTokenManagerImpl.kt' . || true
echo "== locate kotlin files around org/siloserver/silo/network =="
git ls-files | rg 'EncryptedTokenManagerImpl|profile|ProfileRepository|ServerRegistry|switchTo|selectProfile' || trueRepository: Silo-Server/silo-android
Length of output: 13719
🏁 Script executed:
#!/bin/bash
set -u
echo "== target file outline =="
ast-grep outline shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt --view compact || true
echo "== target file relevant sections =="
wc -l shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt
sed -n '1,340p' shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt
echo "== selectProfile definition && call sites =="
rg -n -C8 '\bsel ectProfile\s*(' --iglob '*.kt' . || true
echo "== ensureCacheMatchesRegistryLocked call sites =="
rg -n -C3 'ensureCacheMatchesRegistryLocked\s*\(' --iglob '*.kt' . || true
echo "== direct switchTo call sites =="
rg -n -C4 '\.switchTo\s*\(|ServerRegistry.*switchTo|switchActiveServer' --iglob '*.kt' shared androidApp androidTvApp android-shared baselineprofile || trueRepository: Silo-Server/silo-android
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -u
file="shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt"
echo "== setProfileIdentity section =="
rg -n -C8 'override suspend fun setProfileIdentity|serverScopedKey(KEY_PROFILE_ID|KEY_PROFILE_TOKEN)'; sed -n '250,305p' "$file"
echo "== next sections =="
sed -n '305,375p' "$file"
sed -n '500,620p' "$file"
echo "== ProfileRepository selectProfile and guard =="
file2="shared/src/commonMain/kotlin/org/siloserver/silo/repository/ProfileRepository.kt"
rg -n -C8 'selectProfile|identityScopeStillHolds|snapshotCurrentScope'; sed -n '1,240p' "$file2"
echo "== selectProfile calls focused =="
rg -n -C4 'selectProfile\s*\(' ./androidApp ./androidTvApp ./android-shared ./shared/src --iglob '*.kt' \
| rg -n -C4 'selectProfile\s*\(' | head -n 200
echo "== token manager direct switchTo and switchActiveServer focused =="
rg -n -C4 '\b(serverRegistry|registry)\.switchTo\s*\(' ./androidApp ./androidTvApp ./android-shared ./shared/src --iglob '*.kt' | head -n 220Repository: Silo-Server/silo-android
Length of output: 2297
🏁 Script executed:
#!/bin/bash
set -u
files=(
"shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt"
"shared/src/commonMain/kotlin/org/siloserver/silo/repository/ProfileRepository.kt"
)
echo "== EncryptedTokenManagerImpl matching methods and context =="
rg -n -C6 'getProfileIdentity|getProfileId\s*\(|getProfileToken\s*\(|setProfileIdentity|saveTokensForScope|getAccessTokenForScope|identityScopeStillHolds' "${files[@]}" --iglob '*.kt'
echo "== EncryptedTokenManagerImpl line ranges =="
python3 - <<'PY'
from pathlib import Path
p=Path("shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt")
lines=p.read_text().splitlines()
targets={
"getProfileId": None, "getProfileToken": None,
"getProfileIdentity": None, "setProfileIdentity": None,
"snapshotCurrentScope": None, "saveTokensForScope": None,
"getAccessTokenForScope": None,
}
for i,l in enumerate(lines,1):
for t in targets:
if ("suspend fun "+t) in l or t+"(" in l or t == "setProfileIdentity":
targets[t]=i
print({"targets": targets, "total": len(lines)})
for t, i in targets.items():
if i:
lo=max(1,i-10); hi=min(len(lines),i+40)
print(f"\n--- {t} lines {lo}-{hi} ---")
for n in range(lo,hi+1):
print(f"{n:4}: {lines[n-1]}")
PY
echo "== ProfileRepository selectProfile and guard =='
python3 - <<'PY'
from pathlib import Path
p=Path("shared/src/commonMain/kotlin/org/siloserver/silo/repository/ProfileRepository.kt")
text=p.read_text()
for needle in ["selectProfile"]:
idx=text.find(needle)
print(f"\n--- {needle} first occurrence around byte {idx} ---")
if idx!=-1:
print(text[max(0,idx-1200): idx+1800])
PY
echo "== selectProfile call sites with line context, limited =="
rg -n -C4 'selectProfile\s*\(' \
androidApp androidTvApp android-shared shared/src --iglob '*.kt' \
| sed -n '1,260p'
echo "== direct registry.switchTo call sites, limited =="
rg -n -C5 '(\bserverRegistry\s*\.|registry\s*\.|this\.\s*registry\s*\.|TokenStore\s*\.|tokenManager)\s*switchTo\s*\(' \
androidApp androidTvApp android-shared shared/src --iglob '*.kt' \
| sed -n '1,260p'Repository: Silo-Server/silo-android
Length of output: 24524
🏁 Script executed:
#!/bin/bash
set -u
echo "== TokenManager interface profiles =="
sed -n '1,180p' shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt
echo "== TokenManagerImpl profile methods =="
sed -n '1,220p' shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.kt
echo "== ProfileRepository =="
sed -n '1,220p' shared/src/commonMain/kotlin/org/siloserver/silo/repository/ProfileRepository.ktRepository: Silo-Server/silo-android
Length of output: 20102
Reconcile the cache before writing the profile identity.
setProfileIdentity uses the cached activeServerId, but registry switches can update the live id before the collector refreshes this manager. A write in that window stores server B’s identity under server A’s key. Call ensureCacheMatchesRegistryLocked() first, as the active-server read/write paths do.
🤖 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
`@shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt`
around lines 271 - 290, Update setProfileIdentity so it calls
ensureCacheMatchesRegistryLocked() inside mutex.withLock before reading
activeServerId or writing profile fields. Preserve the existing temporaryScope
guard and subsequent identity persistence logic after the cache has been
reconciled.
| // One read: taking these separately could pair the old profile id with | ||
| // the new profile's token across a switch. | ||
| val profileIdentity = tokenManager.getProfileIdentity() | ||
| val profileId = profileIdentity.profileId | ||
| val profileToken = profileIdentity.profileToken |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'fun isSameSiloHttpOrigin|isSameSiloHttpOrigin\(|cleartextOriginConsent|CleartextOriginNotApprovedException' .
rg -n -C 4 'HttpRedirect|followRedirects|redirect' -g '*.kt' .Repository: Silo-Server/silo-android
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the credential-bearing and cleartext-consent path in AuthInterceptorImpl.
sed -n '45,145p' shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt | cat -n
sed -n '145,235p' shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt | cat -n
sed -n '520,595p' shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt | cat -n
# Inspect cleartext consent helper.
sed -n '1,60p' shared/src/commonMain/kotlin/org/siloserver/silo/network/CleartextOriginConsent.kt | cat -n
# Find plugin/client config for follow-redirects across shared/silo network code.
rg -n -C 3 'followRedirects|HttpRedirect|SiloAuthPlugin|createSiloClient|newHttpClient|install\\(Redirect|Redirect\\(' -g '*.kt' shared | sed -n '1,220p'Repository: Silo-Server/silo-android
Length of output: 16495
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect credential-bearing and cleartext-consent path in AuthInterceptorImpl.
sed -n '1,280p' shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt | cat -n
sed -n '568,595p' shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt | cat -n
# Inspect cleartext consent helper.
sed -n '1,60p' shared/src/commonMain/kotlin/org/siloserver/silo/network/CleartextOriginConsent.kt | cat -n
# Find redirect/client configuration facts without a broken ERE.
rg -n -C 3 'followRedirects|HttpRedirect|SiloAuthPlugin|createSiloClient|newHttpClient|install[[:space:]]*\(Redirect|Redirect\(' -g '*.kt' shared | sed -n '1,260p'Repository: Silo-Server/silo-android
Length of output: 46875
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External
Block credential-bearing requests over cleartext HTTP.
The consent check allows an approved http origin, and the same-origin path then attaches Authorization, X-Profile-Id, and X-Profile-Token. Require HTTPS for requests that carry these credentials. Also ensure followRedirects does not carry them from HTTPS to HTTP or across origins.
🤖 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
`@shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt`
around lines 137 - 141, Update the credential-bearing request flow around
profileIdentity, profileId, and profileToken to attach Authorization,
X-Profile-Id, and X-Profile-Token only for HTTPS requests. Reject or omit
credentials for cleartext HTTP, and configure followRedirects so credentials are
not propagated when redirecting from HTTPS to HTTP or to a different origin.
Two unresolved review findings on PR #168. TvSkylineSectionFeed's return-trip state lived in unkeyed rememberSaveable slots, which are positional. Two feeds composed at the same position in different surfaces — Home and a library detail — share one slot, so a return target armed on one could be restored into the other, sending focus to a card that surface never showed. The saved target carries no owner of its own: it is only [sectionId, itemId, sectionIndex, itemIndex]. The feed now takes a required surfaceKey and keys returnTarget, detailReturnPending and returnGeneration on it. Required rather than defaulted, so a new call site cannot quietly rejoin the shared slot. Home passes "home"; the library detail passes its library id, threaded through RecommendedTab which sits between them. Also replaces variant.enableUnitTest, which AGP 8.10.1 deprecates and 9.0 removes, with the host-tests API. Behaviour is unchanged: the task list still offers only testDebugUnitTest, no release variant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Bounded, observed TV focus acquisition; return focus by stable section and item identity rather than saved indices, which only describe the same card while the data is unchanged; modal and overlay focus ownership; centralised IME handling; and voice search from the remote.
Return restoration handles refreshes, pagination, scrolling, requester attachment and confirmed focus, across Calendar, Collections, Library, People, Personal, Requests and Search. Device pairing now requires resolved lookup data before it will offer a decision, and stale lookup and pagination results are ignored rather than allowed to land late.
Review findings
All three CodeRabbit threads are addressed on the follow-up branch rather than here, to keep this PR at its reviewed size:
HomeViewModelfetch race — overlappingfetchSections()publishers, where an older partial response could replace a newer complete one and mark sections not fully resolved. Closed by afetchGenerationguard.TvSkylineSectionFeedshared saveable slot —rememberSaveableslots are positional, so two feeds composed at the same position in different surfaces shared one, and a return target armed on one could restore into the other. The feed now takes a required surface key.variant.enableUnitTest— deprecated in AGP 8.10.1 and removed in 9.0; replaced with the host-tests API, with the task list still offering onlytestDebugUnitTest.