fix(android): resolve settings end-to-end defects - #146
Merged
Conversation
playback.audio_language and the profile's subtitle_language are BCP 47
language tags in the server's settings contract. The phone put the display
label on the wire verbatim — "English", not "en" — and the TV did the same
for audio while doing it correctly for subtitles.
That was already broken before the server started enforcing it: the same
string is handed to ExoPlayer as preferredAudioLanguage, and
setPreferredAudioLanguage("English") never matches a track tagged eng, so
choosing an audio language on Android has silently been a no-op. It also
meant Android and Apple wrote different vocabularies to the same key —
Apple has always sent codes, so a language picked on an iPhone read as
"Default" on the phone and vice versa.
Now that the server validates the tag, the flusher's PUT 400s and only
logs, so the setting would stop persisting entirely after a server upgrade.
Replaces the four drifted option lists with one table in shared, so a
language cannot be added to one surface and missed on the others, and
translates values already on devices on read rather than re-sending a
label the server will reject.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SettingKeys.kt is generated from the server's manifest by cmd/settingsgen, so this client cannot drift from the contract by editing a constant. The two hand-maintained tables in AndroidPlayerSettingsStore now delegate to it. BOOLEAN_KEYS/INT_KEYS/DOUBLE_KEYS was a second table that had to agree with PlaybackSettingsKeys.DeviceSettings by discipline alone — a key added to one and missed in the other flushes as the wrong type and is silently dropped on read. Only the granular subtitle appearance fields stay local, since the contract carries them as one composite object. A new contract test caught two real drifts, both of which are the disagreements the contract exists to end: subtitle_appearance -> playback.subtitle_appearance. Every other key carries a domain prefix; this one never did. player.next_up_prompt_seconds -> playback.next_up_prompt_seconds. Android shipped player.* while Apple and the server used playback.*, so the same preference was two settings and neither client could read the other's. Both are wire-format changes with no dual-write, which is what the coordinated cutover is for. Part of the cross-platform settings contract (Silo-Server/silo-server#479). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The clients adopted the generated contract bindings but still speak only the legacy string-only settings registry; nothing calls the canonical /settings/contract or /settings/values routes. This adds that surface to SettingsApi, mirroring the server's settings_values.go shapes exactly: - getContractCapabilities() probes /settings/contract/capabilities and returns a sealed SettingsCapabilitiesResult. A 404 means the server predates the canonical API entirely, so it is a typed ServerUpgradeRequired case the UI can present as "this server needs an upgrade" instead of an empty settings screen. - getEffectiveValues(keys, libraryIds, seriesIds) hits the batched /settings/values/effective resolution: typed JSON values, the source scope of each answer, constraint reporting (value vs stored_value), and the contract revision the resolution was computed at. - putValue/deleteValue address one scope explicitly via a validated SettingScopeIdentity: scope + library_id/series_id ride the query, the profile and device identity ride the session headers the auth interceptor already attaches (with a setDeviceSetting-style optional profileId override), matching how the server derives the identity so one profile cannot write another's settings by naming it. - Writes carry X-Silo-Mutation-Id from newSettingMutationId(): one UUID per logical write, held across retries, so the flusher's retries replay the recorded receipt instead of re-applying, and a reused id with different content surfaces as 409 mutation_id_conflict. Wire models live in SettingValueModels.kt beside the legacy models; response scope/source fields stay raw strings so a server that adds a scope cannot break deserialization. Shared unit tests cover the serialization round-trips, the query/header/body encoding, and the upgrade-required mapping for both routeless (plain-text) and JSON 404s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…al API
ServerSettingsFlusher now writes to PUT/DELETE /api/v1/settings/values/{key}
at scope=profile_device with values encoded as the contract's JSON types
(classified by the generated SettingKeys.BOOLEAN_KEYS/INT_KEYS/DOUBLE_KEYS
sets; subtitle appearance goes up as its JSON object, and an empty language
tag as JSON null because the server's language_tag validator rejects "").
The 750ms debounce semantics are unchanged.
The old failure handling was a named defect: a failed PUT logged at Log.w
and dropped the write, so any server hiccup silently turned settings
non-persistent. Now a transient failure (network, 5xx, 408/429/401) keeps
the op queued and retries it with the SAME mutation id — minted once per
logical write via newSettingMutationId() — so the retry is an idempotent
replay the server can dedupe, first on a capped backoff and after that on
the next enqueue/flushNow trigger. Only a response that proves retrying is
pointless (contract rejection, mutation-id conflict) drops the op, and
every failure is logged at warning level with the key and status through
SiloLog. A delete answered 404 not_found is treated as already done.
Non-contract keys (the granular subtitle.* fields Android flattens out of
the composite appearance object) never reach the server, where they would
404 as unknown_setting.
AndroidPlayerSettingsStore.refreshFromServer() now hydrates from the
batched GET /settings/values/effective: typed JSON parsed per the generated
type sets, and a key nothing is stored for arrives as the contract default
with source "default" — so defaults come from the contract, never from a
hardcoded fallback, and a value reset from another device snaps back on
refresh. The subtitle device-override flag now derives from the resolved
scope (profile_device) instead of the legacy has_device_override field. A
key absent from the response means the server's contract predates it, so
the local value is kept. resetAllDeviceSettings deletes only server-stored
keys.
Part of the canonical settings API adoption; the API surface itself landed
in the previous commit.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…opes
The subtitle triple (language, mode, forced) and the metadata language rode
named columns on PUT /profiles/{id}. The server still accepts them, but every
server-side reader resolves those preferences canonically from
user_setting_values, so the column write only takes effect via the mirror the
server keeps until cutover. Android now writes them itself, at scope=profile,
one key per edit — a failed write no longer reverts the other two, which is
what sending the whole triple every time did.
Reads come from the batched effective endpoint rather than the profile object,
so a value set on another device, or narrowed by policy, is what the screen
shows. Both apps go through one shared ProfileSettingsController: this repo's
history has the TV screen missing behaviors the phone has, and a behavior that
lives in one class cannot be present on one platform only.
Quality becomes the two axes the contract actually stores —
playback.preferred_quality (resolution) and playback.max_bitrate_kbps
(bandwidth, null = uncapped) — behind one preset picker whose table is a port
of the web client's qualityPresets.ts. Presets stay client-side on purpose:
retuning what "1080p High" means is a client release, not a contract break.
The compound legacy spellings ("1080p-high") are dead and never written; a
stored one is decomposed on read, dropping the bitrate it encoded rather than
inventing a cap the user never chose.
Subtitle appearance keeps its granular subtitle.* fields client-local (the
contract carries one composite object and would refuse them as unknown_setting)
but they are no longer stranded there: they project into
playback.subtitle_appearance on flush, so a per-field edit reaches the server,
and a resolved appearance flattens back into them so the overlay cannot
resurrect the value the server just replaced.
A server that predates the canonical settings API 404s the contract probe.
Both settings screens now say so instead of rendering rows whose edits
silently go nowhere; playback keeps working from the device-scoped defaults.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The settings contract names four resolvers that must agree: Go in internal/settingsresolve, TypeScript in web/src/lib, Swift in the Apple clients, and Kotlin here. Three of them ran the shared conformance fixture; Kotlin did not, so nothing caught this client resolving a setting differently from the server until a user saw the wrong value. Vendors contracts/settings/v1/conformance.json byte-identically, plus the manifest it was authored against. The manifest is needed because the generated SettingKeys bindings carry key names and a coarse type table but not the facts resolution turns on — resolution_order, default_value, enum member order with its `ordered` flag, and constrained_by. Copying those into Kotlin by hand would recreate exactly the drift the contract exists to remove, so the runner parses the manifest and is driven by it. No generator change is required. The resolver lives in test sources on purpose. Android does not resolve settings in production: it writes through /settings/values and reads effective values back, leaving the server the single authority. This exists so the fixture has a fourth independent implementation to disagree with, which is what makes it a drift gate rather than a tautology. Four things fail the suite, each of them drift: a resolution disagreement, a revision mismatch across the fixture / vendored manifest / generated bindings, a key those two JSON files disagree about (which catches them being vendored from different server commits — skew the revision check cannot see), and any fixture field the runner does not recognize. The last one is why decoding is strict: a field one platform reads and another silently skips means the platforms have stopped running the same cases, and a silent skip is indistinguishable from a pass. Verified by mutating the resolver and confirming the suite fails: reversed resolution order, a null bitrate slipping past a ceiling, a floor capping an unbounded value, allowlist falling back to the definition default, locked narrowing an already-equal value, ordered enum ranking disabled, and foreign-profile rows resolving. Each gate was mutation-tested too. One mutation survives — dropping the non-empty device-id guard — because no fixture case makes it load-bearing in any language; that gap is documented at the guard and is fixed upstream in the fixture, not here, so all four runners gain the case together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Six defects found reviewing the canonical settings adoption, five of which
lose or misreport a user's setting.
A transiently-failed flusher op was re-queued even after a newer value for
the same key was drained and sent in the same flush. `retryable` was
add-only, so a later drain pass that landed a newer op left the older
failed entry behind, and the post-loop `composite !in pending` guard could
not compensate — the pass that sent the newer op had already cleared
`pending`. `scheduleRetry` then replayed the superseded value with its
original mutation id, which the server's first-use-id path does not dedupe,
overwriting the edit the user had just made. Reachable from every
`flushNow()` caller (activity onStop, logout, the device-setting resets),
where a concurrent enqueue is not cancelled. Dropping the composite from
`retryable` on success keeps only the latest failed state per key.
The phone playback starter still read `user_profiles.subtitle_language`.
The settings screens write these preferences at `scope=profile` now, and
nothing on the server mirrors a canonical write back into that column, so
the phone auto-selected subtitles from the pre-edit value while Android TV
— which reads WatchDetail's server-resolved `effective_*` fields — played
the new one. Same intent, same server, different playback per platform.
The phone starter now prefers `effective_*` the way the TV starter does,
and passes the mode and forced-subtitle flag it previously dropped.
The TV detail page's "Auto" subtitle preview had the same stale source:
it advertised the pre-edit preference while starting playback from that
same row used the canonical one. It resolves through
ProfileSettingsController now, translating the snapshot's "" (no
preference) into the preview's null so an unset language does not read as
"no subtitles".
A 404 on the capabilities probe was read as "server too old". That route
sits behind the viewer-access middleware, which answers a JSON
`{"error":"not_found"}` when the X-Profile-Id we send names a profile the
household deleted elsewhere — so a current server told users to go ask
their admin for an upgrade when the fix was re-selecting a profile. A
genuinely old server has no `/settings/contract` routes and gets chi's
plain-text 404, which parses to an empty error code, so gating on that
separates the two.
The TV legacy-prefs import wrote only the resolution axis, leaving a
(resolution, no bitrate) pair no picker preset covers: the row read
"720p" but the picker showed nothing selected with the cursor on Auto, and
the sentinel is marked on the same pass so it could never be re-migrated.
It now writes both axes at the bitrates the server's own migration assigns
the same legacy values.
The only test for the subtitle-appearance projection passed with the whole
feature reverted — it asserted a negative that any no-op satisfies. It now
writes a granular slot through the legacy-import path (the genuinely
unguarded one) and asserts the flush carries it, with a second test for the
read overlay and the redundant-write guard kept separately. Verified by
mutation: deleting either half of the projection now fails.
Every fix is pinned by a test that fails without it, checked by reverting
each change in turn. Full suites green: 2996 tests across shared,
android-shared, androidApp and androidTvApp.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two places collided. Settings screens (AndroidPlayerSettingsStore, PlaybackSettings, SettingsViewModel, SubtitleSettings, TvSettingsScreen, TvSettingsViewModel). Main's PR #125 (invite claim + onboarding tour) landed on the same files this branch moved onto the canonical settings contract. Resolved as a union: the canonical-settings plumbing wins for settings behavior — ProfileSettingsController resolves the profile-scoped keys instead of reading preference columns off GET /profiles, the two-axis QualityPresets picker replaces the single defaultQuality label, per-key optimistic writes replace the whole-triple UpdateProfileRequest PUT, and the SERVER_UPGRADE_REQUIRED notice stays — while main's onboarding/tour additions come across untouched. Main's language-row migration in the profile-load path is dropped as dead code on this branch, not as a reverted intent: those values now come from the effective-values endpoint, which never carried the legacy display labels. The DataStore read still runs LanguageOptions.migrateLegacyValue, so the on-device legacy rows main was protecting are still translated. LanguageOptions. Main revised the file this branch introduced (TAGS -> tags, preservable-tag echo in label(), canonicalSubtitleLanguage-backed migrateLegacyValue). Main's revision is kept wholesale and our call sites are adapted to it: TvSettingsScreen's AudioLanguages/SubtitleLanguages become audioLanguages/subtitleLanguages, and SubtitleSettings collapses its two identical option lists into main's single hoisted languageOptionLabels. migrateLegacyValue is now wider, not narrower — a valid tag outside the picker table ("nl", "pt-BR", "eng") passes through instead of being erased, and "Off" and "Default" still clear. Nothing the flusher tests assert changed. Verified: :shared:testDebugUnitTest (960 tests) and :android-shared:testDebugUnitTest (1021 tests) pass with no failures or skips; :androidApp:compileDebugKotlin, :androidTvApp:compileDebugKotlin, :androidApp:compileDebugUnitTestKotlin and :androidTvApp:compileDebugUnitTestKotlin all build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Eight findings from the Codex and CodeRabbit passes over the canonical settings adoption, six of which lose or misreport a user's setting. The settings cutover renamed two keys (subtitle_appearance -> playback.subtitle_appearance, player.next_up_prompt_seconds -> playback.next_up_prompt_seconds). That is a contract question for the server, but on disk it orphans values an installed build already wrote. Both keys read local-first — subtitle appearance drives downloaded playback with no server in the loop, next-up prompt falls back to its 30s default — so an upgrade silently reverted both. PlaybackSettingsKeys carries the rename table now and the store copies each slot forward once, under its own sentinel: the existing one is already marked on every device that has run a scoped build, so a pass gated on it would never run for the installs actually holding the orphans. A value already under the new name always wins. A queued flusher op outlived a server switch. The flusher is application-scoped and SettingsApi requests are relative, so a retained retry addressed whichever server was active when it was finally sent — and a restored or cloned server recognizing the same profile id would accept it. Ops carry the server they were authored against and are dropped, not deferred, once that origin is no longer active. The bandwidth half of the quality choice never reached playback. The server applies the cap only from the request's bandwidth_cap_kbps and nothing on the playback path reads the stored setting, so "1080p Low" streamed at whatever bitrate the ladder picked. Both starters send it now and the attempt carries it, so replans re-send it rather than silently lifting the limit mid-session. A successful PUT stores the authored value; it does not make it effective. Policy can narrow a setting and a profile_device row outranks the profile row these setters write, so both screens could show a preference playback was not using. ProfileSettingsController re-resolves after each successful write and returns what the server actually holds; a failed re-resolve keeps the optimistic value rather than rolling back a change that landed. The TV legacy import guarded only the resolution axis while setQuality writes both, so a device with a server-side bitrate cap and no resolution override had that cap overwritten by the legacy preset's bitrate — or by JSON null for a legacy Auto. Both axes are queried and guarded. Blank effective_* strings reached subtitle auto-selection as a real preference. A canonical row holding JSON null unmarshals to "" server-side and arrives present-but-empty, which both auto-selectors read as an explicit "subtitles off" — turning subtitles off for users who never chose a language. Normalized on every rung, matching the audio path. Metadata language rendered its unset value as "Off" on both platforms, though it means "inherit the library's language" rather than disabling anything. Verification: :shared, :android-shared, :androidApp and :androidTvApp unit tests plus both app compiles, --rerun-tasks to defeat stale caches — 3173 tests, 0 failures. Not reproduced: CodeRabbit flagged AndroidPlayerSettingsStoreTest:535-543 as a critical compile failure on a nullable smart cast. kotlin.test .assertTrue declares a returns()-implies contract, so the cast holds; the file compiles clean under --rerun-tasks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR standardizes subtitle synchronization and overlay settings on canonical APIs, adds retained external-route delivery with player-target matching, preserves explicit player route intent across reloads, updates profile/avatar contracts, disables Watch Together surfaces, and adjusts several Android and TV layouts. ChangesAndroid behavior updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5525c37b15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5c9211520
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+170
to
+171
| if (replaceCurrentPlayer) { | ||
| popUpTo(Route.Player.ROUTE) { inclusive = true } |
There was a problem hiding this comment.
When a different silo://play request arrives while Player is on top, this inclusive pop clears the old PlayerViewModel, whose onCleared() calls the process-scoped sessionLifecycle.stopAsync() without an expected session ID. Because the replacement ViewModel starts loading asynchronously, that stale stop can acquire the lifecycle mutex after the new session has been adopted and tear down the newly requested playback. Scope teardown to the old session ID or ensure it completes before starting the replacement.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/OverlayPrefsStoreTest.kt (1)
299-343: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider avoiding a live
HttpClientin the fake.
SettingsApi(HttpClient())allocates a real engine per test and never closes it; every method is overridden so no request is ever made. IfSettingsApican take a lazily-constructed or injected client, prefer that (or close the client in a teardown) to keep the unit suite free of engine/thread allocation.🤖 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/settings/OverlayPrefsStoreTest.kt` around lines 299 - 343, Update RecordingOverlaySettingsApi so the unit-test fake does not eagerly allocate an unclosed live HttpClient; use an injectable or lazily constructed client supported by SettingsApi, or ensure the created client is closed during test teardown. Preserve the fake’s existing overridden behavior and avoid creating any request engine when no inherited API method is used.androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt (2)
38-79: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueQuery parsing is not URL-decoded and truncates values containing
=.
substringAfter('=', "")keeps only the text up to the first=, and no percent-decoding happens, so any route argument that was encoded (or contains=) parses to a different value than the one the player resolved fromNavBackStackEntry.arguments. Today's arg set (ids, quality tokens, numeric indices) is safe, but a future encoded arg would silently break exact-target matching rather than fail loudly.♻️ Suggested tightening
- .associate { part -> part.substringBefore('=') to part.substringAfter('=', "") } + .associate { part -> + val name = part.substringBefore('=') + val rawValue = part.substringAfter('=', "") + name to java.net.URLDecoder.decode(rawValue, "UTF-8") + }🤖 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/ExternalRouteNavigation.kt` around lines 38 - 79, Update playerRouteIntentOrNull query parsing to URL-decode parameter names and values, and preserve the full value after the first '=' so encoded arguments and values containing '=' match NavBackStackEntry.arguments exactly. Keep the existing validation and MobilePlayerRouteIntent construction unchanged.
155-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the route constant over the
"invite_claim"literal.Every other route in this file is referenced through
Route(Route.InviteClaim.ROUTEis already inpreAuthenticationDestinationRouteson Line 136). A raw prefix here silently stops matching if that route string is ever renamed.♻️ Proposed change
- val isPreAuthenticationTarget = route.startsWith("invite_claim") + val isPreAuthenticationTarget = route.startsWith(Route.InviteClaim.ROUTE.substringBefore('?'))🤖 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/ExternalRouteNavigation.kt` at line 155, Update the isPreAuthenticationTarget check to use the existing Route.InviteClaim.ROUTE constant instead of the hardcoded "invite_claim" prefix, preserving the current startsWith behavior.
🤖 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.
Nitpick comments:
In
`@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/OverlayPrefsStoreTest.kt`:
- Around line 299-343: Update RecordingOverlaySettingsApi so the unit-test fake
does not eagerly allocate an unclosed live HttpClient; use an injectable or
lazily constructed client supported by SettingsApi, or ensure the created client
is closed during test teardown. Preserve the fake’s existing overridden behavior
and avoid creating any request engine when no inherited API method is used.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt`:
- Around line 38-79: Update playerRouteIntentOrNull query parsing to URL-decode
parameter names and values, and preserve the full value after the first '=' so
encoded arguments and values containing '=' match NavBackStackEntry.arguments
exactly. Keep the existing validation and MobilePlayerRouteIntent construction
unchanged.
- Line 155: Update the isPreAuthenticationTarget check to use the existing
Route.InviteClaim.ROUTE constant instead of the hardcoded "invite_claim" prefix,
preserving the current startsWith behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9aea5cff-dda7-45d1-8e61-1507b52171df
📒 Files selected for processing (41)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SiloPlaybackService.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/VideoPlayerMediaSpec.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStore.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/OverlayPrefsStore.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/PlayerSettingsStore.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStoreTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/OverlayPrefsStoreTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/ServerDrivenConfigRefresherTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/SubtitleSyncOverridesTest.ktandroidApp/src/androidMain/AndroidManifest.xmlandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/detail/ItemDetailScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobilePlayerRouteTarget.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerOverlay.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/CreateProfileScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/CreateProfileViewModel.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/EditProfileScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/EditProfileViewModel.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileAvatar.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/MobilePlayerRouteTargetTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/profiles/AvatarOptionsTest.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/auth/TvPairDeviceScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvCreateProfileScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvCreateProfileViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileFormOptions.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/testing/FakePlayerSettingsStore.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/feature/ClientSurfacePolicy.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/profile/ProfileModels.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/PlaybackSettingsKeys.ktshared/src/commonMain/kotlin/org/siloserver/silo/overlays/OverlaySchema.ktshared/src/commonMain/kotlin/org/siloserver/silo/overlays/OverlayTypes.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/feature/ClientSurfacePolicyTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/profile/ProfileQualityPreferenceTest.kt
💤 Files with no reviewable changes (9)
- androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvCreateProfileViewModel.kt
- android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/ServerDrivenConfigRefresherTest.kt
- androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/testing/FakePlayerSettingsStore.kt
- shared/src/commonMain/kotlin/org/siloserver/silo/model/profile/ProfileModels.kt
- shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/PlaybackSettingsKeys.kt
- androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.kt
- androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileFormOptions.kt
- android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/SubtitleSyncOverridesTest.kt
- android-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStore.kt
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.
Summary
This PR fixes nine defects found during end-to-end Android phone and TV emulator testing of #119:
player.subtitle_sync_ms. Phone and TV setters, HUD state, persistence, and the playback service now share the same profile-device value, with active media reparsed so changes apply during playback.silo://playdelivery one-shot and lifecycle-safe. External routes are retained with monotonic identities, back-stack observation ends before navigation, identical active Player targets are idempotent, and different Player targets replace the current entry with a fresh playback owner.MainActivitynow usessingleTop.ui.card_overlaysreads, writes, and resets. Failed optimistic writes restore the last confirmed value, and focused tests assert that no legacy endpoint is called.preset:dicebear:...references and remove the stale TV raw-emoji catalog.quality_preferencepayload from both phone and TV profile creation.Profile-creation quality safety
For #142, this intentionally takes the safe option allowed by the issue: remove the creation-time UI and legacy payload rather than fabricate best-effort canonical writes after profile creation.
Profile creation and canonical quality persistence are not one transaction, and a follow-up write could leave a successfully created profile with only part of its two-axis quality preset persisted. Quality remains configurable through the canonical settings path after creation.
Stack dependency
This PR is intentionally stacked on #119 and targets
fix/language-tag-wire-values.Base reviewed:
34767aa2a1ee416a7758b4c38277a591b5785c04. Review this PR as the changes after that commit. #119 should land first, after which this PR can be retargeted or merged towardmain.Verification
Clean phone/TV unit tests and APK builds:
./gradlew --no-daemon \ -Dorg.gradle.jvmargs='-Xmx6g -Dfile.encoding=UTF-8' \ -Pkotlin.compiler.execution.strategy=in-process \ --max-workers=1 \ :androidApp:testDebugUnitTest \ :androidApp:assembleDebug \ :androidTvApp:testDebugUnitTest \ :androidTvApp:assembleDebug \ --console=plainResult:
BUILD SUCCESSFUL in 2m 30s; 152 tasks, 12 executed.Emulator verification:
Issues
Closes #137
Closes #138
Closes #139
Closes #140
Closes #141
Closes #142
Closes #143
Closes #144
Closes #145
AI Disclosure
gpt-5.6-solMaintainer review remediation
Addressed both follow-up review threads in
fe8e3b40:silo://playdelivery against the destination-scoped live player target while preserving automatic versus explicit file, quality, audio, subtitle, and resume intent.Verification:
BUILD SUCCESSFUL in 4m 8s(152 tasks).Pill -> Square -> Pillpersisted across re-entry/foreground refresh and was restored; identical play-link redelivery preserved the active player. TV shared overlay hydration loaded a populated catalog after foreground refresh. No crash, ANR, native fault, or relevant network error was found.AI-assisted maintainer remediation: Codex, model
gpt-5.6-sol, for implementation, review, and verification.Summary by CodeRabbit
New Features
Bug Fixes
Changes