Skip to content

fix(tv,phone): focus ownership, return focus, and voice search - #168

Merged
RXWatcher merged 34 commits into
Silo-Server:mainfrom
RXWatcher:codex/focus-ownership-fixes
Aug 6, 2026
Merged

fix(tv,phone): focus ownership, return focus, and voice search#168
RXWatcher merged 34 commits into
Silo-Server:mainfrom
RXWatcher:codex/focus-ownership-fixes

Conversation

@RXWatcher

@RXWatcher RXWatcher commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

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:

  • HomeViewModel fetch race — overlapping fetchSections() publishers, where an older partial response could replace a newer complete one and mark sections not fully resolved. Closed by a fetchGeneration guard.
  • TvSkylineSectionFeed shared saveable slotrememberSaveable slots 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 only testDebugUnitTest.

Note: this description was rewritten after the original was accidentally overwritten while folding in follow-up work. The scope and commits are unchanged; only the prose is new.

RXWatcher and others added 11 commits August 4, 2026 15:57
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>
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The 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.

Changes

Android IME and TV focus

Layer / File(s) Summary
Shared IME and stock keyboard handling
androidApp/src/androidMain/..., androidTvApp/src/androidMain/...
Nullable IME callbacks preserve Compose defaults. TV surfaces hide the stock IME when forms and dialogs leave composition.
TV focus acquisition and modal ownership
androidTvApp/src/androidMain/.../focus/*, androidTvApp/src/androidMain/.../components/*
Initial focus uses bounded retries and observed focus state. Modal boundaries suppress covered controls and restore focus after dismissal.
TV return restoration and search
androidTvApp/src/androidMain/.../focus/*, androidTvApp/src/androidMain/.../screens/*
Return targets use stable identities across refreshes and pagination. Search adds voice input, keyboard handling, and lifecycle-based focus restoration.

Playback and identity state

Layer / File(s) Summary
Audio selection and playback teardown
android-shared/src/androidMain/.../player/*, androidApp/src/androidMain/.../screens/player/*, androidTvApp/src/androidMain/.../screens/player/*
Audio selections use catalog ordinals and semantic mounted-track matching. Teardown uses session ownership gates. Subtitle and recovery failures report their actual outcomes.
External routes and device pairing
androidApp/src/androidMain/.../navigation/*, androidApp/src/androidMain/.../MainActivity.kt
Routes carry identity scope, validate server origins, prevent duplicate consumption, and requeue pairing links through authentication gates.
Profile identity and asynchronous ViewModels
shared/src/commonMain/..., shared/src/androidMain/..., androidApp/src/androidMain/.../profiles/*
Profile ID and token updates are atomic and scope-guarded. Stale lookup, profile, home, and pagination results are discarded.

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
Loading

Possibly related PRs

Suggested reviewers: quick104

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 29.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the pull request’s primary focus, including TV and phone focus handling, return-focus restoration, and voice search.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 value

Use camelCase for focus-budget properties.

These properties start with Tv, so they do not use camelCase.

  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookOverlay.kt#L50-L61: Rename the three TvAudiobookPanelFocus* properties to names that start with tv.
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.kt#L92-L95: Rename the two TvAudiobookTransportFocus* properties to names that start with tv.

As per coding guidelines, use camelCase for 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

📥 Commits

Reviewing files that changed from the base of the PR and between a7d1b74 and 38b21f0.

📒 Files selected for processing (40)
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/components/SiloKeyboardActions.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/components/aurora/AuroraChrome.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/AuthComponents.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/DevicePairingScreen.kt
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/components/SiloKeyboardActionsTest.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvDialogInitialFocus.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvFilterSheet.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvStockImeLifecycle.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvTextInputDialog.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvContentInitialFocus.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvControlEnablement.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvModalFocusOwnership.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvPairDeviceFocusTarget.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvProfileFocusTarget.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookBookmarksPanel.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookChaptersPanel.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookOverlay.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookSkipIntervalPanel.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookSleepPanel.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookSpeedPanel.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookTransportRow.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/auth/TvPairDeviceScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/auth/TvServerSetupScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/browse/TvBrowseScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/calendar/TvCalendarScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/collections/TvCollectionDetailScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/collections/TvCollectionsScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/collections/TvCreateCollectionDialog.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvAiTranslateDialog.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/search/TvSearchScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/servers/TvServerListScreen.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/components/TvStockKeyboardPolicyTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvContentInitialFocusTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvPairDeviceFocusTargetTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvProfileFocusTargetTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/calendar/TvCalendarFocusRoutingTest.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/DevicePairingViewModel.kt
  • shared/src/commonTest/kotlin/org/siloserver/silo/viewmodel/DevicePairingUiStateTest.kt

RXWatcher and others added 17 commits August 5, 2026 01:04
…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>
RXWatcher and others added 2 commits August 5, 2026 01:07
…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>
@RXWatcher RXWatcher changed the title fix(tv,phone): focus ownership and reachability follow-ups fix(tv,phone): focus ownership, return focus, and voice search Aug 4, 2026
@RXWatcher

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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>
@RXWatcher
RXWatcher force-pushed the codex/focus-ownership-fixes branch from b2e3c6e to 9c9c869 Compare August 4, 2026 23:46
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>
@RXWatcher

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Use runTest’s scheduler for the queued coroutine.

PersonalListViewModel launches work with viewModelScope, so creating a separate TestCoroutineScheduler here is unnecessary and adds a second virtual clock. Use StandardTestDispatcher(testScheduler) and advance testScheduler in 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 win

Consider retrying the shelf focus request before reporting the token consumed.

DayShelf calls targetCardFocusRequester.requestFocus() exactly once, then calls onFocusApplied(). onFocusApplied() clears shelfFocusDay, shelfFocusRequest, and shelfFocusItemIndex, 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 out TvFocusAcquisitionBudgetMillis and then falls back to the shell handoff.

The flat surfaces use requestFocusUntilObserved with 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 value

Supply descriptive messages to awaitState calls.

kotlin.test.assertTrue is already imported, so this concern does not affect compilation. The description parameter 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

📥 Commits

Reviewing files that changed from the base of the PR and between 38b21f0 and 9f1d5e1.

📒 Files selected for processing (27)
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/browse/CatalogLetterIndexViewModelTest.kt
  • androidTvApp/build.gradle.kts
  • androidTvApp/src/androidMain/AndroidManifest.xml
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvCatalogGrid.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvSkylineSectionFeed.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvFlatReturnRestoration.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvReturnAdapters.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/focus/TvReturnTarget.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/calendar/TvCalendarScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/collections/TvCollectionDetailScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/library/TvLibraryDetailScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/people/TvPersonDetailScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/personal/TvPersonalScreens.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/requests/TvMyRequestsScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/search/TvSearchReturnProjection.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/search/TvSearchScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/search/TvVoiceSearch.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvReturnAdaptersTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/focus/TvReturnTargetTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/library/TvLibraryFocusRestoreTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/search/TvSearchReturnProjectionTest.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/DevicePairingViewModel.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/PersonalListViewModels.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/RequestsViewModels.kt
  • shared/src/commonTest/kotlin/org/siloserver/silo/viewmodel/DevicePairingDecisionOrderingTest.kt
  • shared/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

Comment thread androidTvApp/build.gradle.kts
…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>
@RXWatcher

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Stop retries after every focus-owner change.

targetState returns Disposed only for a different non-null focusedProfileId. ProfileTileGrid reports null for Add Profile and when no profile tile owns focus. If the viewer moves to Add Profile while retries run, the state remains Ready, 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 win

Track list materialization separately from successful focus anchoring.

tvProfileFocusTarget uses hasMaterialized to identify the first list arrival. This code passes hasAnchored, which remains false when the first request is cancelled because the viewer moves focus. On the next profile-list change, the helper returns currentIds.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

📥 Commits

Reviewing files that changed from the base of the PR and between 9f1d5e1 and ec0c3ca.

📒 Files selected for processing (7)
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvSkylineSectionFeed.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/home/TvHomeScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/library/TvLibraryDetailScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionScreen.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/DevicePairingViewModel.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/HomeViewModel.kt
  • shared/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

@RXWatcher RXWatcher changed the title fix(tv,phone): focus ownership, return focus, and voice search fix(tv,phone): focus, playback identity, profiles and navigation Aug 6, 2026
@RXWatcher
RXWatcher force-pushed the codex/focus-ownership-fixes branch from 0320a67 to ec0c3ca Compare August 6, 2026 00:52
@RXWatcher RXWatcher changed the title fix(tv,phone): focus, playback identity, profiles and navigation fix(tv,phone): focus ownership, return focus, and voice search Aug 6, 2026
@RXWatcher
RXWatcher merged commit 65ca623 into Silo-Server:main Aug 6, 2026
3 checks passed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Security And Privacy (CWE-74): Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection')

Reachability: External · Exploitability: Moderate

Percent-encode contentId in TV playback route builders.

contentId comes from uri.pathSegments.lastOrNull() for launcher deep links. TvRoute.Player, ItemDetail, and AudiobookPlayer interpolate contentId directly, so values containing ? or & can inject query parameters. Percent-encode each contentId path 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 win

Fold eac3_joc into the eac3 family.

normalizedAudioToken strips the underscore, so a catalog codec eac3_joc becomes eac3joc. No branch matches it, and the else returns eac3joc. A mounted Media3 track reports audio/eac3, which canonicalises to eac3. The codec filter in matchMountedAudioTrack then 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.toAudioMimeType in this PR already treats eac3_joc as 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 lift

TV has no bounded fallback for a local audio switch that never takes.

The phone implementation bounds this branch with MAX_LOCAL_AUDIO_ATTEMPTS and then calls replanForDesiredAudio, because AudioTrackManager.selectAudioTrack returns Unit, does nothing when the group has gone, and produces no callback. The TV Apply branch reissues LocalAudioSelection on 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 value

Stacked 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 onto onPlaybackRecoveryExhausted and the "The screen has shown ... subtitleFailureMessage" block onto onSubtitleFailureShown; keep only the audio-commit block on onAudioSelectionCommitted.
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt#L959-L972: delete the first, superseded KDoc block so desiredAudioOrdinal carries 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, not mobileAudioTrackPersistenceUpdate; move it onto selectedServerAudioTrackIndex or 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 win

Make setProfileIdentity() overlay-safe or document the expected behavior.

The default call to setProfileId() then setProfileToken() 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 win

Use 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} requires camelCase for 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 win

The 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 calling castReturnFocus.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 pendingCastFocusIndex alone does not stop the loop, because the loop reads castRestoreFocused and castReturnFocus rather than the index. The cast branch needs an ownership check like the stillOwned() 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 value

Consider surfacing malformedSets.

malformedSets is 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 own SubDiag.log line, 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 win

Bind the unused OutOfMemoryError to _ to satisfy detekt.

detekt reports SwallowedException at Line 240. The Exception branch above logs e::class.simpleName and e.message; this branch binds e and never reads it. Rename the binding to _, which matches the catch (_: EOFException) style already used in this file.

The decision to catch OutOfMemoryError here 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 to publishDisplaySet.

♻️ 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 win

Remove the shadowed output local at Line 208.

flushDisplaySet takes an output: TrackOutput parameter. Line 208 declares a local val output = trackOutput ?: return with the same name. The local shadows the parameter, and the parameter is never read afterwards. Kotlin reports NAME_SHADOWING for this, which fails the build if the module enables allWarningsAsErrors.

The re-read also adds no safety. The single caller at Line 148 already resolved trackOutput at Line 105, and trackOutput is assigned only in init.

♻️ 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) return

Then 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 win

Add a positive assertion so the test cannot pass vacuously.

The test has no assertion. It relies only on runTest reporting an uncaught exception. The test therefore also passes if stopAsync never invokes stopSession at 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

📥 Commits

Reviewing files that changed from the base of the PR and between ec0c3ca and 0320a67.

📒 Files selected for processing (89)
  • android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/MediaAuthSession.kt
  • android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.kt
  • android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackTeardownGate.kt
  • android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/audio/DelayAudioProcessor.kt
  • android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/backend/VideoPlaybackBackend.kt
  • android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractor.kt
  • android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/SubripPayloadNormalizer.kt
  • android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/AudioReconcile.kt
  • android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/EpisodeSelectionHandoff.kt
  • android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/MountedAudioMatching.kt
  • android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/PlaybackStartupStallDetector.kt
  • android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/PostResumeVideoStallDetector.kt
  • android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/network/WatchTogetherRealtimeWebSocketTest.kt
  • android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.kt
  • android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/audio/DelayAudioProcessorTest.kt
  • android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractorTest.kt
  • android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/subtitle/SubripPayloadNormalizerTest.kt
  • android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/AudioReconcileTest.kt
  • android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/MountedAudioMatchingTest.kt
  • android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/MountedAudioReorderTest.kt
  • android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/PostResumeVideoStallDetectorTest.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/PushNotificationPresenter.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/BottomNavBar.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkRoutes.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatch.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRoute.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/MainScreen.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/DevicePairingWrongServerScreen.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleAutoSelection.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlaybackStatsSheet.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerScreen.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/PINEntryDialog.kt
  • androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationAttributionTest.kt
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkEncodingTest.kt
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParserTest.kt
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatchTest.kt
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRouteTest.kt
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/performance/MobilePlayerLifecyclePerformanceSourceTest.kt
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/MobileAudioTrackSelectionTest.kt
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleAutoSelectionTest.kt
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapterTest.kt
  • androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.kt
  • androidTvApp/build.gradle.kts
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvSkylineSectionFeed.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAudiobookRouting.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvRoute.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvAudiobookDetailHero.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailViewModel.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvPlaybackFormatting.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/home/TvHomeScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/library/TvLibraryDetailScreen.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerHud.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerScreen.kt
  • 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/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvVideoPlaybackStarter.kt
  • androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvItemDetailNavigationTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvPlayerRouteTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvPlaybackFormattingTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/PlayerTrackEntriesTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitleIntegrationPolicyTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleSettlementOwnershipTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleTransactionAdapterTest.kt
  • androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvWatchTogetherSurfaceSourceTest.kt
  • shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt
  • shared/src/androidUnitTest/kotlin/org/siloserver/silo/network/EncryptedTokenManagerScopeGenerationTest.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/model/playback/SubtitleTransition.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/model/profile/ProfileModels.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/network/PlaybackRealtimeClient.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/repository/ProfileRepository.kt
  • shared/src/commonMain/kotlin/org/siloserver/silo/viewmodel/HomeViewModel.kt
  • shared/src/commonTest/kotlin/org/siloserver/silo/network/SiloAuthPluginPinTest.kt
  • shared/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

Comment on lines +37 to +45
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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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=kotlin

Repository: 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 || true

Repository: 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 || true

Repository: 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 || true

Repository: 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 androidApp

Repository: 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.

Comment on lines +393 to +402
/** 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),
)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Comment on lines +350 to +366
// 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,
),
)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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/player

Repository: 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.

Comment on lines +2975 to +2990
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)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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=kotlin

Repository: 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.kt

Repository: 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.kt

Repository: 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=kotlin

Repository: 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' . || true

Repository: 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.

Comment on lines 3961 to 3967

// 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Comment on lines 633 to 639
override suspend fun start(request: VideoPlaybackStartRequest): VideoPlaybackStartResult =
suspendCoroutine { continuation ->
synchronized(pending) {
requestCount.value = synchronized(pending) {
pending += Pending(request, continuation)
pending.size
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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.

Comment on lines 233 to 241
// 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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)
PY

Repository: 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}")
PY

Repository: 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.

Comment on lines +3485 to +3489
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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.

Comment on lines +271 to +290
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()
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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=kotlin

Repository: 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' || true

Repository: 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 || true

Repository: 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 220

Repository: 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.kt

Repository: 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.

Comment on lines +137 to +141
// 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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.

RXWatcher added a commit that referenced this pull request Aug 6, 2026
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant