Integrate the fork: subtitles/PGS, Watch Together, security hardening, player and sync fixes - #108
Closed
RXWatcher wants to merge 92 commits into
Closed
Integrate the fork: subtitles/PGS, Watch Together, security hardening, player and sync fixes#108RXWatcher wants to merge 92 commits into
RXWatcher wants to merge 92 commits into
Conversation
Combines the client performance/stability program with the transactional subtitle coordinator, on top of the merged diagnostics work (Silo-Server#94). Performance and stability: - Bounded startup home hydration and coalesced home section requests. - Isolated mobile/TV playback clocks to stop whole-screen recomposition. - Lifecycle-safe player teardown; playback preserved across recreation. - Non-blocking playback exit, with final resume position owned by an application-scoped, auth-scoped FinalPlaybackPositionWriter. - Detail enrichment paused off-route; bounded similar-item hydration. Transactional subtitles (mobile + TV): - Typed subtitle identities replace fingerprint matching, with reserved server/downloaded/local namespaces and reorder-safe downloaded IDs. - Staged replans with atomic commit; the previous session is never stopped until a validated replacement commits. - Load-owner and mutation fencing so stale or superseded loads cannot adopt. - Ordered committed-selection persistence and TV HUD/refresh/remount settlement. Integration notes where both lines touched the same paths: - awaitPendingStop() applies to the legacy adoptActiveSession wrapper only; applying it to the transactional adoptActiveSessionIfCurrent serialized adoption against the stop it supersedes. - Final-position durability uses the application-scoped writer rather than a runBlocking(NonCancellable) teardown backstop, so exit never blocks main. - Exit keeps the non-blocking stopAsync structure, with the subtitle flush ordered ahead of session stop. Verified: 3033 unit tests across shared, android-shared, androidApp and androidTvApp with 0 failures and 0 skips; phone and TV debug APKs assemble.
Two multi-agent audits (a subtitle mount forensics pass and a top-to-bottom client audit) produced 41 verified defects; all are fixed here with regression tests. 3062 unit tests pass. Subtitle mount/replan chain (TV): - Media3 prefixes a merged sidecar's Format.id with its MergingMediaSource child index, so exact-equality matching never matched and the mount timed out. - POST /replan is a server-side commit: rolling back an in-place replan retired a plan the server still held, so every later replan was rejected 409. - Mount authority (Auto < Restore < UserTransaction): a rollback-armed Off could evict a selection the user had just applied. - A mount confirmed during a settlement was discarded; settlements could leak deferred work into unrelated settlements. Session lifecycle: - A cancelled outage-recovery job still published a terminal Failed state over a session that was playing fine. - A deferred stop() could kill a session another screen had already adopted. Downloads, auth, persistence: - MediaStore RELATIVE_PATH used "Downloads"; MediaProvider only accepts the singular "Download", so every ebook download failed permanently on API 29+. - Pairing wrote the approved server's tokens into the previous server's slot. - Cancelling a download never cleared its Room sidecar, permanently disabling the download button for that file. - Downloaded multi-part audiobooks never persisted a position. Plus safeApiCall no longer folds caller cancellation into NetworkError, which was load-bearing in three separate defects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013weYphCSEi4M9c48yVEsqJ
…dinator An adversarial review of the audit work found that several of its own fixes were incomplete. These are the ones inside the subtitle coordinator. The plan cursor did not survive a commit. commitStagedVideoReplan rebuilds the attempt from the PRE-request snapshot and installs it, discarding the advance written when the server accepted the plan — so the cursor fix moved the dead end from the second subtitle pick to the third rather than removing it. A committed replan makes the rendered plan the server plan, so the cursor is now cleared there and the plain planId addresses correctly. The revert helper could also graft a cursor across session identities, permanently retiring a plan the server never issued for the session still playing. The skip-compensation branch — added so a settlement would not tear down a mount that had actually succeeded — returned without draining the settlement, leaving the HUD on "Applying", the choice unpersisted, and a deferred reset stranded for an unrelated settlement to pick up. It now settles as a confirm. The deferred fresh-preference restore was unreachable: the load path resets and restores for the same content one line apart, and the drain dropped the restore whenever a reset was pending, so a saved preference was silently ignored on any reload that landed mid-settlement. It is now keyed to the content it was captured for, applied after the reset, and applied directly so it cannot re-defer on the settlement being drained. Includes the full review as docs/notes, since this branch is parked: it records what remains (R5, R6, R8-R11, S1, S9), the device matrix none of this has run, and that 723 of 3062 tests assert by grepping Kotlin source text. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013weYphCSEi4M9c48yVEsqJ
…dings S1: both seek paths still addressed the server by the RENDERED plan, so after a subtitle mount failed and rolled back — the routine outcome this work exists to handle — seeks silently did nothing. Recovery now addresses the plan the server holds; re-anchor declines outright once the two have diverged, because it requires the response to come back on the rendered plan and could only 409; and both adopt paths clear the cursor, without which an accepted recovery re-opened the 409-forever bug. All request builders now go through one serverPlanId accessor, since this is the second time a call site was forgotten. R11: auto-selection consumed its one-shot flag before the priority gate could refuse the emit, so on a retry path preferred-subtitle auto-enable never ran again for that ViewModel — silently, and the flag is never reset. Also ports the three fixes already reviewed on the ship branch: the profile- pinned settings delete, the cast identity reclaim gating on ownership rather than presence, and the download sidecar resurrection window. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013weYphCSEi4M9c48yVEsqJ
… works Down from clean playback reveals the transport row, which is what Jim asked for and what silo-apple does. But the escalation half of that change -- a second Down with the overlay up opening the HUD -- was answered in the root preview handler, which sits above the scrubber in the tree. It consumed the key, so TvPlayerScrubber's own Down handler never ran and onMoveDownToTransport became dead code. silo-apple has no such escalation. TVPlayerScrubber answers .down with onMoveToTransport, and TVPlayerTransportCluster answers .up only (`default: break`) -- there is nothing below the transport row. The HUD is reached by the Tune button in that row (slider.horizontal.3 on tvOS) or by Menu/Settings. Both entry points already exist here, so the HUD is not stranded. Down is therefore the overlay's own business once the overlay owns focus: both the root bridge and the idle-overlay handler now fall through, and the scrubber gets its hand-off back. Also restores the tvOS mid-scrub trap the hand-off needs. tvOS ignores Down while a scrub is in flight -- it is nearly always drag spillover -- and additionally pulls the transport buttons out of the focus graph (TVPlayerTransportCluster.allowsFocus). We have no such switch, so the scrubber consumes Down instead; falling through would let Compose move focus to the transport row and drop the scrub mid-adjustment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013weYphCSEi4M9c48yVEsqJ
…for embedded tracks Four register items, three of which were fixes that did not do what they said. R8 — the TV's stop-ownership guard was inert. Every exit path runs prepareSessionExit, which blanks uiState.sessionId, and onCleared read the id afterwards, so all three stops passed null and the guard never engaged. The id is now latched before the blanking and the two remaining exit paths name their session too, stopSessionForExit most of all: it awaits settlement first, so the next item can already be playing by the time it stops. S7 — that guard also read SessionState, which carries a session id only while Active. During Reconnecting a stale stop found no id, fell through, and cancelled the recovery: the outage banner vanished with nothing replacing it and progress reporting for that episode was dead for the rest of playback. Ownership is now a token independent of the presented state. stopAsync takes the id too, and no longer coalesces onto an in-flight stop that targets a different session, which would have dropped the newer stop entirely. R5 — the recovery guard treated a deferred publication as proof that ownership had moved. A subtitle commit leaves one for up to 30s, which outlasts the 10s progress interval, so a blip inside that window entered Reconnecting holding our own pending publication. Recovery then refused to run, and because beginOutageRecovery's Reconnecting guard blocks later attempts and settlePendingPublicationIfCurrent requires Active, the banner could never clear. Only a publication for a *different* session is evidence. S9 — picking an embedded (muxed) subtitle while another mutation was still committing replanned the stream instead of switching the track locally: black flash, rebuffer, re-seek, to arrive at the same picture. The direct press and the queued replay had drifted apart, so the local-commit step is now shared. It declines when the folded pending also carries an audio, quality or output-route preference, since only the server can apply those — the direct press had that hole in the other direction and this closes both. Tests: each fix has a check that fails against the previous line. The two lifecycle tests are executable, not source greps. Also tightens the two `sessionLifecycle.stop(` needles the review flagged as weakened to a prefix of the pre-fix code, and pins the latch-before-blank ordering that made R8 inert. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013weYphCSEi4M9c48yVEsqJ
…branch does Same concept, two names across the two branches — the drift that produced S9 in the first place. Aligning now keeps the eventual merge honest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013weYphCSEi4M9c48yVEsqJ
…th the ship branch S2 — libass paints ASS itself and emits no cues, so OffsetSubtitleParserFactory never sees them and the renderer clock is the only place the sidecar source-timeline delta can be applied. Feeding it the user-only offset meant an ASS/SSA sidecar on a resume-into-transcode showed dialogue from near the start of the file under the current scene, then went blank past the last event — while SRT on the same stream was perfect. Already correct on the ship branch; this brings the coordinator in line. S6 — cherry-picked: backoff was reordering an item's outbox ops, so a retried mark-watched could wipe a newer resume position. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013weYphCSEi4M9c48yVEsqJ
S5 — the outbox is scoped by (serverId, profileId) and the only thing keeping a
backed-off scope's retry chain alive is SyncWorker's Result.retry(), driven by
what SyncEngine counts as remaining for the scope active at the END of a drain.
A profile switch leaves activeServerId untouched, and nothing else re-triggers a
drain, so the chain for the scope that still had queued work simply stopped:
watched marks, ratings, favourites and resume positions from that profile never
synced again. On a TV, which is never relaunched, permanently and invisibly.
The starter now watches the (server, profile) pair rather than the server alone.
S4 — removing a server wiped four token keys and the registry entry and nothing
else. Reclaiming space reclaimed none, and since idFor() is base64 of the
normalised URL, re-adding the same server resurrected its Downloads list, resume
positions, cached home/catalog rows and pending outbox ops under an identity the
user believed they had deleted. Two DAOs had deleteAllForServer with no callers;
five tables had no delete-by-server query at all.
The purge is an observer, not a step inside remove(): remove() runs inside
identityTransitions.changing { mutex.withLock { … } } on Main, and a multi-GB
filesystem walk there would serialise every identity mutation behind it —
besides which AndroidServerRegistry lives in `shared` and has no Room access.
Orphans are derived (rows with no registry entry), so the pass is idempotent,
re-runnable on every cold start, and cleans installs that removed servers before
this existed. Downloads are cancelled before their rows are deleted, or a live
DownloadWorker writes its row back on the next progress tick; a server whose
file is currently playing is deferred whole, so offline PiP is not cut off
mid-scene.
Both fixes have tests that fail against the previous line. S5's run on real
dispatchers deliberately — the starter's collector lives on its own scope and
advanceUntilIdle does not drive it, so a virtual-time test observes nothing at
all and passes for the wrong reason.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013weYphCSEi4M9c48yVEsqJ
…p accumulating
S3. ass-kt frees native memory only in finalize(), and embedded MKV font
attachments accumulate in the ASS_Library the process-global AssHandler owns.
Anime and typeset releases carry 5-30 MB of attachments each, so native memory
grew monotonically for the process lifetime — ending as a mid-episode decoder
allocation failure or a silent process kill. "The app just dies after a couple
of episodes."
The handler is now dropped when the player it belongs to is replaced, which
makes the whole graph collectable, fonts included. Everything derived from it
had to move at the same moment or the retired handler stays reachable and
nothing is reclaimed:
- the parser factory now delegates through the current handler, because
SiloPlayerFactory wraps it once and holds those wrappers for the process
lifetime;
- the AssSubtitleView overlay is removed from its host, because attachTo
reuses any overlay already there and that view holds the handler it was
built with — reuse across a recycle would render from a handler no player
drives.
Deliberately NOT Ass.clearFont(): libass permits ass_clear_fonts only once
every track and renderer on that library is released, and AssHandler merely
nulls its Kotlin references on a media-item transition. Calling it is a
use-after-free risk and is lossy on resume-at-offset mounts.
NO EXECUTABLE COVERAGE. The bridge's constructor probes the native runtime,
which is absent under JVM unit tests, so renderingSupported is false and the
entire recycling path is inert there — the existing LibassBridgeTimeTest only
exercises static arithmetic. This is device-validation gated: play two
font-heavy MKVs back to back and watch native memory.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013weYphCSEi4M9c48yVEsqJ
Two hygiene items the review flagged as must-decide-before-any-PR. SubDiag: the call sites stay — they are the only observability into the mount/replan chain, and several of this branch's defects are diagnosable from nothing else. What must not ship is the output: the lines interpolate subtitle identities and session ids, and this is a public client. Now off unless `adb shell setprop log.tag.SUBDIAG DEBUG`, gated inside the helper so all thirty call sites are covered by one check rather than thirty that can be half-applied. Also drops from Log.e — diagnostic chatter should not masquerade as an error or pollute crash breadcrumbs. Letterbox: LetterboxProbe opened the transcode stream a SECOND time to sample frames, which is what made the server reject replans with 409. It has no callers, and leaving it plus green tests invites someone to re-wire it. Deleted, along with the frame-scanning detectLetterboxInsets/pixelLuma it was the sole caller of, its ten tests, and SubtitleManager.applyLetterboxInsets — the entry point it would have called, which no production code ever did. Note the review was wrong that all three files were unreferenced: LetterboxInsets and insetByLetterbox are load-bearing in SubtitleManager's video-rect maths, so those stay. The inset is now always NONE, exactly as it already was at runtime. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013weYphCSEi4M9c48yVEsqJ
# Conflicts: # android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SiloPlayerFactory.kt
…ture list PR Silo-Server#99 added the DIRECT_STREAM_RESUME_V1 capability to the client feature set but did not update this expectation, so upstream/main fails this test on its own — verified on a pristine checkout of 27de43a. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013weYphCSEi4M9c48yVEsqJ
Both changes landed on ship/client-defect-fixes (f9888e3) but never reached the coordinator line, and neither file was touched on main since the branches diverged at Silo-Server#94, so these are exact ports. - SiloCastOverlay: when SiloCastState.loadError is set the receiver never started the stream, so the transport controls would drive a stream that never plays. Show the failure and offer the one action that recovers — ending the session hands playback back to the local player. - AndroidServerRegistry: purge every key in the server's "<serverId>." namespace instead of a fixed key list, so per-server credential slots added later can't linger encrypted on disk after the server is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
main and ship/client-defect-fixes fixed the same reordering bug in different places. main clamps a backed-off sibling forward at enqueue time (enqueueCoalescingRestoringItemOrder, 05df9c3); ship enforced per-item FIFO in the drain itself (f9888e3). The enqueue-time clamp only survives the first failure: when the older op fails a SECOND time, recordFailure pushes it back into the future, dueBatch stops returning it, and the newer op drains ahead of it anyway — so a retried SET_WATCHED still lands after a newer SET_POSITION and clears the resume position the user just created. Take ship's drain-level guarantee: read every pending row for the scope, and hold an op back while any older op for the same content id is still queued or backing off. Unrelated items keep draining in parallel. The enqueue-time clamp stays — it still shortens the first retry — but correctness no longer rests on it. Adds the regression test for the second-failure state, which fails on main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AndroidServerRegistry decodes its entire state from one JSON blob and mapped both "no blob" and "blob would not decode" to an empty registry. That was survivable while an empty registry only meant "sign in again": ids are derived from the normalised URL, so the rows and downloaded bytes stayed on disk and re-adding the server restored them. OrphanedServerDataPurger changed the stakes. It derives orphans as "in the database, absent from the registry" and runs at startup, so an unreadable blob presents every server as an orphan — cancelling downloads, deleting the files and dropping every row, including queued outbox ops. One corrupt preference became permanent data loss. Distinguish the two cases at the source and expose the failure on the registry interface, then have the purger decline to act on it. Absence is only evidence of removal when the registry knows what it holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
start() ran its API call outside the lifecycle mutex, and for that whole window _state is Loading with lastAdoptedSessionId null — so stop()'s ownership guard found no id to compare against and tore down anyway. Two consequences. The teardown nulls lastStartParams, and startInternal never re-set it, so the session that then published Active had none: 404 session recovery and the final progress flush both silently no-op, losing the user's resume position on exit. And the newly started session was never stopped, because the stop that would have owned it had already run — leaving it alive on the server, counting against the account's concurrent-stream cap until it timed out. That is the same shape as the orphaned cast sessions that produced the 429 storm. Publish the start result under the mutex and compare a stop epoch captured before the API call: if a teardown ran while we were starting, stop the new session and stay Idle instead of resurrecting a dismissed screen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…next start Recycling only happens when a different player initializes, so the last player used stayed referenced for the process lifetime — along with its AssHandler and every embedded font that handler accumulated, 5-30 MB for a typeset release. Leaving the player back to browsing is exactly when that should be reclaimed, and where users park longest. Add releasePlayer() to the bridge and route both ExoPlayer teardown sites through SiloPlayerFactory so the handler goes with the player it belonged to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…indow The 90s window was enforced against an estimate reconstructed from the retry count, which sums only the delays between attempts. Time spent inside each failed attempt was invisible, so against a server that accepts connections and then stalls — 30s socket timeout per try — the real elapsed time ran several times the window before an error surfaced, leaving a spinner up for minutes. Measure from the first error of each load task and take whichever bound is larger. Callers that cannot measure keep the derived behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…start The manager and the lifecycle each hold their own pending-publication record and are assumed to move together, but the manager's is created inside startVideoSessionV3 while the lifecycle's is installed by the caller afterwards. Cancel the start between the two — back out of the TV player while it is loading — and the manager keeps a publication nothing will ever settle. Both waits on it were unbounded, and the recovery hatch the TV starter runs before every start reports success when the LIFECYCLE has no pending, without ever consulting the manager. So the next start walked past the hatch into beginContentReset and blocked forever: an infinite spinner, for that start and every one after it, unrecoverable without killing the process. Bound the content-reset wait and roll a stale publication back in place, so the manager self-heals whoever the owner was, and have the TV starter clear the manager's side as well as the lifecycle's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mobile commits without deferred publication, so when the adapter abandons a committed playback — superseded adoption, failed adoption, reset during commit, or a lost context — ownership has already moved to that session and the predecessor is being cleaned up. There is nothing to revert to, unlike the TV adapter's joint rollback. It stopped the session but left it installed as the active attempt, so every later replan and progress report addressed a session the server had already torn down. Disown it at the same time and let the lifecycle's session-missing recovery take over. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two findings CodeRabbit raised on PR Silo-Server#97. That PR's code was squashed into main without them being triaged, so both were live here. FinalPlaybackPositionWriter drained `pending` before writing and swallowed the result, so a failed write discarded the user's final position outright — no retry, no re-queue — and this queue is the durable record that survives a server-side session reset. Failures now go back on the queue with a backoff, using putIfAbsent so a newer position submitted meanwhile still wins. PlayerViewModel.onExit did not cancel the Up Next countdown, so a countdown still running when the user left could fire and start an episode behind a dismissed screen — creating exactly the kind of session that outlives the exit meant to end it. Also makes the content-reset self-heal timeout injectable. runTest advances virtual time whenever the scheduler idles, so the fixed 45s value could fire inside tests that legitimately wait for a settlement, which made `new content start waits for unresolved replacement settlement` order-dependent. Tests asserting the wait now pass NEVER_SELF_HEAL; the self-heal test asks for the real value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit 4af0f4d)
(cherry picked from commit 517cc32)
(cherry picked from commit 63cdcad)
(cherry picked from commit 941dee8)
(cherry picked from commit 475a7ba)
…ta-source fake The security branch predates main's entity-change/if-range work, so its copy of AuthenticatedDataSourceFactoryTest carries a different FakeHttpDataSource. Keep main's fake and port only what the content-limit commit actually added: the three SubripNormalizingDataSource limit cases and their ByteArrayDataSource. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The injectable settle timeout used Long.MAX_VALUE as its "never self-heal" sentinel. runTest's virtual scheduler fires such a timeout immediately, so the self-heal ran inside the very tests meant to assert the wait: it rolled back the replacement, the test then rolled it back again, and the session was stopped twice. Only showed up under a full-project run, which is what made it look like pre-existing flakiness. Take a nullable timeout instead — null means await with no timeout at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The shell's top navigation is a TopStart overlay drawn over every screen
rather than something that reserves space, and this screen started its
content at the top of the window — so the title rendered through the nav
row ("Movies" sat on top of "Music Pizza") and the REQUEST eyebrow was
clipped off the top edge entirely. Content now starts below the nav and
inside a TV-safe horizontal margin.
The overview was the full TMDB synopsis, unbounded: several hundred words
that filled a 10-foot display and pushed the Request action off the
bottom. Clamped to four lines — the job here is deciding whether to
request something, not reading the plot.
Adds the backdrop the screen always had the artwork for, behind a
horizontal and a vertical scrim so body copy stays legible over a busy
frame, with the poster alongside the text. Title, metadata and tagline
are bounded too, so a long title cannot push the action off-screen again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
Hosting pre-selects the title, so a new room is playing-and-selected the instant it exists — and every routing decision keyed on exactly that. Creating a room therefore flashed the invite code for a fraction of a second and replaced it with the player, leaving nothing on screen to read out to the person you were inviting. A host who is still alone now stays in the lobby, where the code is shown large, and the lobby hands off by itself once the member count rises. A guest is unaffected: they join a room that is already running and should land in it rather than be stranded behind a code they just used. Applied at both decision points on both platforms — the entry routing (TvAppNavigation, watchTogetherDestination) and the lobby's own auto hand-off (shouldEnterSyncedPlayer, lobbyPlayerDestinationOrNull) — since either alone would still skip past the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
The QR had no quiet zone at all. MARGIN was set to 0 with a comment saying the surrounding Box supplied padding instead, but the canvas was the same size as the box, so the modules ran edge to edge — and a 16dp rounded clip then cut the corners off the finder patterns, the three squares a scanner locks onto. Against a dark TV background there was nothing separating the code from the wall either. Square corners now, with the spec's clear border. The lobby was a single non-scrolling Column. The host block alone — code, QR, two cycler rows, close button — is taller than a 1080p panel once the suggestion list sits under it, so "Guest controls" and everything below it were drawn past the bottom edge and could not be reached at all. It scrolls now, with bottom padding for a focused row's scale inside the overscan-safe area. The suggestion list moves out of a LazyColumn while I am here: a lazy list nested in a scrolling column is measured with an unbounded height and crashes. A room's suggestions are few enough that laziness bought nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
Making the lobby scroll fixed the unreachable controls but broke the one thing the screen exists for: moving focus down to the cycler rows scrolled the join code off the top, so a host looking at the controls could no longer read the code out to the person they were inviting. Split into two columns. The invite — code and QR — is pinned on the left and never scrolls. Everything focusable lives in the right column, which scrolls on its own, so the code stays put however far down the list you go. The right half of the screen was empty before this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
…ything # Conflicts: # android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/AudiobookPlayerStartPositionTest.kt # android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackCapabilityDetectorSourceTest.kt # android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackV3SessionTest.kt # android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/SiloPlayerFactorySubtitleParserTest.kt # android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/backend/VideoPlaybackBackendInterfaceTest.kt # androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/performance/MobileDetailOpenPerformanceSourceTest.kt # androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerScreenStartPositionTest.kt # androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelSharedCoordinatorTest.kt # androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/reader/ReaderEngineHostSourceTest.kt # androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/components/TvSkylineSectionFeedSourceTest.kt # androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/performance/TvDetailOpenPerformanceSourceTest.kt # androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvDetailPlaybackSelectionSourceTest.kt # androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/home/TvHomeScreenSourceTest.kt # androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerControlsUsabilityTest.kt # androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerScreenStartPositionTest.kt # androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModelPlaybackPositionTest.kt # androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModelSharedCoordinatorTest.kt # shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.kt
|
Important Review skippedToo many files! This PR contains 327 files, which is 227 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (328)
You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The server now decides vote rooms by the tally, but nothing on the client could create one or act on it: both apps called createRoom with no mode, so every room came back host_pick, and the lobby showed vote counts that changed nothing. Hosting now offers "Host a vote room" alongside the ordinary one — selection_mode is fixed at creation server-side, so it has to be chosen there rather than switched on later. In a vote room the lobby marks the leading suggestion WINNING and gives the host a single "Start winner" action above the list, labelled with the title it will start. It is disabled until something has a vote, because the server refuses that start and a button that can only fail is worse than one visibly not ready. The per-row "Pick" is hidden in vote rooms. The server promotes only the winner, so offering it on every row would be an action that fails on all but one of them. roomVoteWinner lives in shared and mirrors the server's rule exactly — head of the vote-ordered list, nothing when no votes are cast — so the client's idea of who is winning cannot drift from the server's and leave a host staring at a refusal they cannot explain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
Both clients created a room and immediately set the title the host was looking at as its selection. In a vote room that sets the room playing before anyone can suggest anything: everyone is thrown into the player and the vote never happens, so the mode was dead on arrival. A vote room now opens with no selection and lands the host in the lobby, where suggestions and votes are collected. host_pick is unchanged — it still pre-selects, because there the host's choice IS the selection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
A vote room had no way to acquire candidates. addSuggestion existed on the repository and the API and was called from nowhere on either platform, so the lobby sat on "No suggestions yet" permanently and the host's Start winner action could never leave its disabled state. The mode was unusable end to end. The TV detail overflow now offers "Suggest to Watch Together" whenever the viewer is actually in a room. Suggesting belongs on item detail rather than in the lobby: you choose something to suggest by browsing for it, and the lobby has no catalogue in it. The room comes from the repository's live snapshot, which outlives any one screen, so the action follows you around the app while a room is open. The result is confirmed with a toast — the dialog closes on selection, so without one the action is indistinguishable from having missed it. Movies previously had no overflow at all; being in a room now brings one, same as the Watch Together entry does. Phone has no equivalent yet. This is also the narrow version of a bigger gap: Watch Together is reachable only from a title, which assumes a room is always about the title in front of you — true for host_pick, wrong for a vote room, where you are deciding what to watch in the first place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
Scaffolding for making a room outlive the screen that opened it. NOT yet wired: the four existing connect() call sites still own their own connections, so behaviour is unchanged by this commit. The connection currently lives in screen scope — the lobby's viewModelScope and, separately, the player controller's scope. Leaving the lobby closes the socket, the server sees the host disconnect, and the room is closed for everyone shortly after. That makes "stay in the room while you browse for something to suggest" impossible, which is exactly what a vote room needs its members to do. It also removes a latent bug rather than only enabling a feature: because both scopes call connect(), navigating lobby -> player runs two reconnect loops racing over the repository's single realtime field. A single owner fixes that by construction instead of by timing. Next: register it on an application scope in both apps' DI, then change the lobby view models and both RoomSyncControllers to observe it rather than connect themselves. That step needs a two-device test before anything is built on top of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
Wires the holder added in 864de27. All four screen-scoped connections are gone: the lobby view models and both RoomSyncControllers now adopt the app-scoped session instead of opening their own. Two things this fixes. A room survives leaving the lobby. The connection used to live in viewModelScope, so backing out closed the socket, the server saw the host disconnect, and the room was closed for everyone. Browsing for something to suggest — the whole point of a vote room — took the room down with it. And a lobby -> player hand-off no longer runs two reconnect loops. Both scopes called connect(), racing over the repository's single realtime field; whichever lost left a dead client believing it was connected. One owner removes that by construction rather than by timing. Leaving is now explicit: only the lobby's leave() drops the connection, never composition ending. The server's host-disconnect grace was raised to two minutes in parallel, so a genuine drop costs a reconnect rather than the room. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
Collaborator
Author
|
Refreshed to current The Watch Together portion of this branch is reviewable on its own as #106, which now carries the same commits and a fuller write-up. Server counterparts: Silo-Server/silo-server#486 (vote enforcement) and #487 (host-disconnect grace). |
Brings the phone to parity with TV. addSuggestion was called from nowhere on either platform, so a vote room could not acquire candidates; TV got its entry point first and mobile was left without one, which meant a phone participant could vote but never nominate. "Suggest to Watch Together" now appears in the movie and series detail overflow whenever the viewer is in a room, and the result is confirmed with a toast — the menu closes on selection, so otherwise the action is indistinguishable from having missed the item. The room comes from the repository's live snapshot, which since the session hoist outlives any single screen, so the action is available wherever you wander while a room is open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
Follows the server: the tally is advisory, so the host may start something other than the winner. The per-row "Pick" returns in vote rooms, where it had been hidden precisely because the server used to refuse it. "Start winner" stays as the one-press path to the tally's answer, and the WINNING badge still shows what the room chose — the host now simply is not bound by it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
Ports silo-apple#95. Televisions crop the outer edge of the picture, and the two subtitle paths were protected very differently. Media3's text layer clears the bottom via setBottomPaddingFraction, but libass places ASS/SSA cues in the script's own coordinate space against whatever frame it is handed — and that frame is the subtitle view, full size. A cue authored at the bottom of the frame was composited into pixels the panel never shows, so the line simply never appeared. No error, no log, just missing dialogue. The inset goes on the surface rather than in either renderer, so both paths inherit it, and it is applied after the letterbox inset so the two compose instead of fighting: bars first (what the source wastes), then overscan (what the panel eats). Styled text keeps its current position. Its bottom padding is a fraction of the now-smaller surface, so the presets give back exactly what the surface took; stacking them would have pushed Media3 cues visibly higher than they sit today. TV only — phones do not overscan and stay at zero. 5% is the long-standing broadcast-safe margin. It is deliberately not a user setting: a viewer cannot tell "cropped by the panel" from "not rendered", so this is not a preference they could reason about. Unverified on a panel. The mechanism is certain; whether 5% is right for a given TV is not, and that needs eyes on the Shield. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
Ports silo-apple#86. Android TV already surfaced Up Next automatically at the credits, but there was no way to reach it early — a viewer who is done with an episode had to sit through the outro or leave the player and navigate back in. The control appears in the transport's secondary group only when a next episode is actually resolved, and the predicate lives on the view model so the manual button and the automatic trigger cannot disagree about whether there is anything to advance to. A button that appears when the automatic path would find nothing is a button that does nothing. It deliberately does NOT start the auto-advance countdown. Someone who opened this themselves is choosing; a timer that pulls them into the next episode mid-decision is the opposite of what the press asked for. The automatic path keeps its countdown and its pass-out gating untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
Ports the decode half of silo-apple#93. A server library of type "mixed" — movies and series in one folder — mapped to no MediaMode, so it never reached navigation, search or browse on either platform. Not degraded: erased, with nothing to indicate a library was missing. Admitting it as a video library is the whole fix for visibility. The TV browse path already does the right thing with it: tvCatalogMediaTypeFor returns null for an unrecognised type, which scopes the browse by libraryId alone rather than forcing a mediaType — exactly what a merged library needs. Not ported: Apple also added a Type facet (Movies / Series) so the merged grid can be narrowed by filtering rather than navigation, and lists the library under both cascade dropdowns on tvOS. That is a UX decision worth making deliberately rather than mirroring blind, and it is worth nothing while the library is invisible. Visibility first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
Collaborator
Author
|
Refreshed to current
The title-safe fix has no standalone PR because it builds on the letterbox/PGS subtitle work, which only exists on this branch — it does not apply to |
CI failed dependency verification on this branch: guava-parent-33.4.8-jre.pom had no entry, so :android-shared:debugUnitTestRuntimeClasspath refused to resolve. The artifact arrived transitively with work merged upstream today, after this branch's metadata was last generated. It built locally because the artifact was already in the Gradle cache — the check only bites on a cold runner, which is exactly where it should. This is the supply-chain hardening working, not misfiring: an unverified artifact entering the build is precisely what it exists to stop. The fix is to record the hash, not to weaken the check. Generated with --write-verification-metadata sha256; the diff is the one missing component and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Db4dSxN9tH8yN7uUP549tK
This was referenced Jul 27, 2026
Collaborator
Author
This was referenced Jul 27, 2026
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
Everything that has accumulated on my fork's
mainand never been proposed upstream — 80 commits, 320 files. It is merged with currentupstream/main, builds clean, and the full unit suites pass onshared,androidAppandandroidTvApp.I would rather have sent this as a dozen focused PRs, and I tried. It does not decompose: the history contains an earlier upstream integration merge, several cross-cutting refactors (the player coordinator, the libass handler lifecycle, the subtitle transaction model) that later commits build directly on, and a shared-model rename resolved in this merge. Cherry-picking themes out of it produces branches that do not compile. Splitting it honestly would mean rewriting the work, not reorganising it.
So: take it as an integration branch, or tell me which slices you want carved out and I will rebuild those properly rather than pretend a
git cherry-pickdoes it.What is in it, by theme
Subtitles and PGS (~14 commits). A
.supextractor so bitmap sidecars can be framed at all (Media3 parses PGS but only one display set at a time, timestamped by a container that a raw.supdoes not have). Client-side PGS mounting instead of forcing server burn-in. SDH picks matching their own artifact. Per-item subtitle sync, a burn-in guard, a selectable CC row, in-stream captions named rather than numbered. Appearance defaults aligned to the web client. Letterbox insets so cues sit over the picture on sources with baked-in bars — this is the client half of Silo-Server/silo-server#483.Watch Together (7). The feature was fully built and completely unreachable: no entry point existed on TV at all, and the phone's was behind a const. Opening it exposed six defects, including client→server frames vanishing silently when the socket was not yet up, which cost the room
attach_sessionfor its whole lifetime. Broken out for focused review as #106.TV request detail (1). Content rendered under the shell's nav overlay. Broken out as #107.
Security and supply chain (~15). Consent before cleartext login; rejecting a scope whose persistent credentials were replaced; identity-scoped download paths and the races around them; ambiguous HTTP authority rejection; keeping credentials on the Silo origin; authenticated PiP actions; epub sanitisation with a parsed allowlist, SVG paint validation, bounded remote/archive content, webview isolation; dependency verification metadata, pinned native provenance, fail-closed OSV parsing.
Player and sync correctness (~20). Sessions that finish starting after the user leaves; unsettled publications wedging future starts; failed final-position writes; libass handlers accumulating embedded fonts; outbox ordering; draining on profile change; purging removed servers.
TV player and picker fixes (~10), plus test infrastructure (~5) — including a recorded emulator sweep.
The merge
Non-trivial, and worth a look:
PlaybackSourceV3→PlaybackSourceDescriptorV3and addeddurationSeconds; my fork had addedcolorRangeand the letterbox fractions to the old type. The auto-merge produced a data class with twosourcefields. Resolved to a single type on upstream's name carrying all the fields.PlaybackApiTest: upstream replaced a hand-maintained feature list withPLAYBACK_START_CLIENT_FEATURES_V3. Took upstream's.Caveats, stated plainly
TvWatchTogetherSurfaceSourceTestin the Watch Together work is a source-grep test — the pattern fix(ci): repair PlaybackApiTest and drop source-grep pseudo-tests #102 just removed. It exists because the failure mode was absence of wiring, which nothing behavioural could observe while the dialog was unreachable. Happy to drop it if the policy is absolute.AI-use disclosure
Written by Claude Opus 5 (Claude Code) working with @RXWatcher, who directed the work, reviewed it, and ran the device testing throughout.