fix(settings): send language tags, not display names - #119
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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughThe change adds typed canonical settings APIs, profile-scoped resolution, two-axis quality presets, subtitle appearance projection, Android synchronization with retries, and updated Android/TV settings and playback integrations. ChangesCanonical settings platform
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsUI
participant SettingsViewModel
participant ProfileSettingsController
participant SettingsRepository
participant SettingsApi
SettingsUI->>SettingsViewModel: select preset or subtitle setting
SettingsViewModel->>ProfileSettingsController: load or write profile preference
ProfileSettingsController->>SettingsRepository: resolve effective values or persist value
SettingsRepository->>SettingsApi: call typed settings endpoint
SettingsApi-->>SettingsRepository: return typed response
SettingsRepository-->>ProfileSettingsController: return snapshot or result
ProfileSettingsController-->>SettingsViewModel: reconcile or roll back state
SettingsViewModel-->>SettingsUI: render resolved settings
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: 4aeb78b46d
ℹ️ 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".
| _uiState.update { | ||
| it.copy( | ||
| subtitleLanguage = profile.subtitleLanguage?.ifBlank { "Off" } ?: "Off", | ||
| subtitleLanguage = profile.subtitleLanguage.orEmpty(), |
There was a problem hiding this comment.
When an existing profile still contains a value written by the old phone picker, such as "English", this keeps that legacy label in state while the new picker renders it as “Off.” If the user then changes only Subtitle Mode or Show Forced Subtitles, persistProfileSubtitleSettings() sends the unchanged "English" value in the same request, causing the validating server to reject the entire update. Convert legacy profile labels to their tags here, as is done for the audio setting, so existing users retain their selection and can update the other subtitle controls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Obsolete as of the canonical-API commits later in this PR — the line this points at no longer exists.
SettingsViewModel stopped reading profile.subtitleLanguage.orEmpty() entirely; the screen now resolves through ProfileSettingsController against GET /settings/values/effective, and the setters write scope=profile per key rather than sending the subtitle triple as one UpdateProfileRequest. So neither half of the scenario applies: there is no legacy label in state to preserve, and no combined request that a stale field could invalidate.
The underlying concern is handled where the value now enters: LanguageOptions.migrateLegacyValue on the store read path, and server-side migration maps the legacy columns to canonical rows.
| TAGS.any { it.first == stored } -> stored | ||
| else -> wireValue(stored) |
There was a problem hiding this comment.
When another client or the server supplies a valid BCP 47 preference not present in this ten-language UI list, such as nl or en-US, migrateLegacyValue() converts it to UNSET. Since refreshFromServer() stores the server value and audioLanguageFlow passes it through this function, playback silently loses that valid audio preference and the UI reports “Default.” Migration should translate known legacy display names but preserve other valid tags rather than treating the picker allowlist as the complete BCP 47 vocabulary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already handled — migrateLegacyValue preserves valid tags outside the picker list.
The table lists only the ten languages the pickers offer, but the migration falls through to isPreservableTag, which accepts anything BCP 47-shaped that canonicalSubtitleLanguage recognizes. So nl and en-US pass through untouched; only values that are neither a known tag nor a known legacy label become UNSET. label() does the same, echoing an unlisted tag back as itself rather than rendering it as "Default" — labeling an active preference as off would be its own bug.
Pinned by the existing LanguageOptionsTest.
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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.kt (1)
20-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse camelCase property names.
Rename
UNSETandTAGSto camelCase and update callers. 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 `@shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.kt` around lines 20 - 27, Rename the LanguageOptions properties UNSET and TAGS to camelCase equivalents, then update every Kotlin caller and reference to use the new names while preserving their values and behavior.Source: Coding guidelines
shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/SettingKeys.kt (1)
11-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffGenerate camelCase Kotlin properties.
REVISION, the generated key constants, andREMOTE/CLIENT_LOCAL/typed sets violate the Kotlin property naming rule. Updatecmd/settingsgen’s Kotlin template and regenerate rather than editing this generated file. 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 `@shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/SettingKeys.kt` around lines 11 - 196, Update the Kotlin template used by cmd/settingsgen so generated properties use camelCase, including REVISION, key constants, and REMOTE, CLIENT_LOCAL, and typed-set properties; then regenerate SettingKeys.kt. Do not edit the generated file manually, and preserve the generated key string values and membership lists.Source: Coding guidelines
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt (1)
2034-2036: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename these properties to camelCase.
Use
audioLanguagesandsubtitleLanguages, including their call sites. As per coding guidelines,**/*.{kt,kts}requirescamelCasefor 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/settings/TvSettingsScreen.kt` around lines 2034 - 2036, Rename the Kotlin properties AudioLanguages and SubtitleLanguages to audioLanguages and subtitleLanguages, respectively, and update every reference to these symbols across their call sites while preserving their existing LanguageOptions configuration.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
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.kt`:
- Line 136: Update the subtitleLanguage assignment in the profile-to-UI-state
mapping to pass the stored value through LanguageOptions.migrateLegacyValue(...)
before applying the empty fallback. Preserve the existing handling for null or
empty values while ensuring legacy labels such as “English” are normalized to
the shared language contract.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SubtitleSettings.kt`:
- Around line 11-12: Update the metadata language handling in SubtitleSettings,
including metadataLanguageOptions and its corresponding label(...) usage, so the
empty value is displayed as “Default” rather than “Off.” Keep the subtitle
language picker labeled “Off,” and use the same “Default” label consistently for
metadata options and labels.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/PlaybackSettingsKeys.kt`:
- Around line 9-13: Add a one-time local migration before settings reads and
flushes: copy values from the pre-cutover subtitle-appearance and next-up prompt
identifiers to PlaybackSettingsKeys.SubtitleAppearance and the corresponding
next-up prompt key, then remove the old entries. Apply the change at
shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/PlaybackSettingsKeys.kt
lines 9-13 and 28-31, preserving existing values when present and avoiding
dual-write behavior.
In
`@shared/src/commonTest/kotlin/org/siloserver/silo/model/settings/SettingKeysContractTest.kt`:
- Around line 53-77: Extend theSharedKeysAgreeOnSpelling in
SettingKeysContractTest with explicit equality assertions for
SettingKeys.PLAYBACK_SUBTITLE_APPEARANCE and
PlaybackSettingsKeys.SubtitleAppearance, and for
SettingKeys.PLAYBACK_NEXT_UP_PROMPT_SECONDS and
PlaybackSettingsKeys.NextUpPromptSeconds. Keep the existing key assertions
unchanged.
---
Nitpick comments:
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt`:
- Around line 2034-2036: Rename the Kotlin properties AudioLanguages and
SubtitleLanguages to audioLanguages and subtitleLanguages, respectively, and
update every reference to these symbols across their call sites while preserving
their existing LanguageOptions configuration.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.kt`:
- Around line 20-27: Rename the LanguageOptions properties UNSET and TAGS to
camelCase equivalents, then update every Kotlin caller and reference to use the
new names while preserving their values and behavior.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/SettingKeys.kt`:
- Around line 11-196: Update the Kotlin template used by cmd/settingsgen so
generated properties use camelCase, including REVISION, key constants, and
REMOTE, CLIENT_LOCAL, and typed-set properties; then regenerate SettingKeys.kt.
Do not edit the generated file manually, and preserve the generated key string
values and membership lists.
🪄 Autofix (Beta)
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: 1d35bfb2-edbb-4d24-bc5a-da3c344abcd4
📒 Files selected for processing (10)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStore.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/PlaybackSettings.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SubtitleSettings.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/PlaybackSettingsKeys.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/SettingKeys.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/settings/SettingKeysContractTest.kt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abf6334ae8
ℹ️ 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".
| // Android shipped this under player.* while Apple and the server used | ||
| // playback.*, so the same preference was two settings and neither client | ||
| // could read the other's. The contract settles on playback.*. | ||
| const val NextUpPromptSeconds = "playback.next_up_prompt_seconds" |
There was a problem hiding this comment.
On an upgrade where Android has already persisted a non-default value under player.next_up_prompt_seconds, changing this constant makes every DataStore read and server refresh use only playback.next_up_prompt_seconds. The existing migration is sentinel-gated and iterates only the new DeviceSettings keys, so it never copies the old slot; if the initial server refresh fails or the server has not migrated that device value, the preference silently reverts to 30 seconds. Read and migrate the old key once before relying exclusively on the new contract key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 34767aa.
You are right that the sentinel-gated migration never reaches these slots — and the reason it cannot is worth stating: ensureMigrated's sentinel is already marked on every device that has run a scoped build, so a rename pass gated on it would never execute for exactly the installs holding the orphaned values.
PlaybackSettingsKeys.RenamedLocalKeys is the table now, and migrateRenamedKeys copies each old slot forward once under its own sentinel. It checks both the scope-prefixed and unprefixed spelling (a value could have been written on either side of the scoped-store change), types the copy by the new key's contract type, and never overwrites a value already present under the new name — a fresh edit or a canonical refresh outranks whatever the pre-rename build left behind.
Worth noting this is not only a stale-cache problem: subtitleAppearanceFlow drives downloaded playback with no server in the loop at all, so offline users would never have gotten the value back.
Pinned by a value stored under a pre-cutover key name survives the rename and the rename migration never overwrites a value already under the new key, plus a contract test asserting the table's targets are still the live contract keys.
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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 112489a114
ℹ️ 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".
| it[stringPreferencesKey(scope.keyPrefix + PlaybackSettingsKeys.PreferredQuality)] = normalized | ||
| it[intPreferencesKey(scope.keyPrefix + PlaybackSettingsKeys.MaxBitrateKbps)] = capped |
There was a problem hiding this comment.
When a user selects a capped preset such as “1080p Low” and starts playback before the 750 ms server flush completes—or whenever that flush is temporarily failing—the new bitrate is only stored in this flow and is ignored by the start request. Both MobileVideoPlaybackStarter and TvVideoPlaybackStarter still read only preferredQualityFlow and send only the resolution as qualityPreference, so playback can exceed the selected bandwidth cap even though the settings UI already displays it. Carry maxBitrateKbpsFlow into the playback request, as the in-player quality path does, rather than relying solely on the eventual server-side write.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 34767aa. Confirmed by tracing the server side: internal/playback/plan_v3.go applies the cap only from the request's bandwidth_cap_kbps, and nothing on the playback path resolves the stored playback.max_bitrate_kbps — so this was not a debounce race, the cap never applied at session start at all, flush or no flush.
Both starters now read maxBitrateKbpsFlow and pass it through to startVideoSessionV3, which sets bandwidthCapKbps. The cap is also stored on ActiveVideoAttempt and re-sent on all three replan paths — otherwise the first recovery would have silently lifted the limit for the rest of the session.
| // Renamed at the settings cutover: every other key carries a domain prefix | ||
| // and this one did not, so the contract registers it as | ||
| // playback.subtitle_appearance. Old servers are gone by the time this | ||
| // ships, so there is no dual-write. | ||
| const val SubtitleAppearance = "playback.subtitle_appearance" |
There was a problem hiding this comment.
On an upgrade, an existing scoped DataStore can still contain the user's composite appearance under subtitle_appearance, but this rename makes subtitleAppearanceFlow read only playback.subtitle_appearance. Because ensureMigrated() is sentinel-gated and iterates only the new key names, it never copies the old slot; until a canonical server refresh succeeds, playback—particularly downloaded playback during an offline or transiently unavailable refresh—falls back to the default appearance despite the device override remaining enabled. Copy the old composite into the renamed local slot during migration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 34767aa.
You are right that the sentinel-gated migration never reaches these slots — and the reason it cannot is worth stating: ensureMigrated's sentinel is already marked on every device that has run a scoped build, so a rename pass gated on it would never execute for exactly the installs holding the orphaned values.
PlaybackSettingsKeys.RenamedLocalKeys is the table now, and migrateRenamedKeys copies each old slot forward once under its own sentinel. It checks both the scope-prefixed and unprefixed spelling (a value could have been written on either side of the scoped-store change), types the copy by the new key's contract type, and never overwrites a value already present under the new name — a fresh edit or a canonical refresh outranks whatever the pre-rename build left behind.
Worth noting this is not only a stale-cache problem: subtitleAppearanceFlow drives downloaded playback with no server in the loop at all, so offline users would never have gotten the value back.
Pinned by a value stored under a pre-cutover key name survives the rename and the rename migration never overwrites a value already under the new key, plus a contract test asserting the table's targets are still the live contract keys.
| fun setSubtitleMode(mode: SubtitleMode) { | ||
| val previous = _uiState.value.subtitleMode | ||
| _uiState.update { it.copy(subtitleMode = mode) } | ||
| persistProfileSubtitleSettings() | ||
| viewModelScope.launch { | ||
| if (profileSettings.setSubtitleMode(mode.wire) !is ApiResult.Success) { |
There was a problem hiding this comment.
When policy locks or narrows a profile setting, a successful PUT stores the user's authored value but does not make it effective; for example, a playback.subtitle_mode lock can keep the effective mode at always after the user selects off. This optimistic update is retained after every successful write without re-resolving the effective value or carrying the response's constrained metadata, so both phone and TV can display “Off” while playback continues using “Always.” Re-fetch the effective setting after a successful write, or expose the constraint and prevent the picker from presenting the authored value as active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 34767aa, and the fix also closes the sibling finding on ProfileSettingsController (thread on line 84) — same root cause.
ProfileSettingsController setters now return a WriteResult carrying a re-resolved snapshot, and both view models apply it instead of keeping the optimistic value. The re-resolve is skipped as a no-op when the server agrees with what the user chose, so a concurrent edit to a different field in the same pane is not clobbered; a failed re-resolve returns a null snapshot and the caller keeps its optimistic value rather than rolling back a write that did land.
That covers both narrowing mechanisms: policy constraint (playback.preferred_quality carries constrained_by.ceiling today, and EffectiveSettingValue has carried constrained/stored_value since the contract landed) and a profile_device row shadowing the profile row these setters write.
Two tests, including one where the resolver answers always for a write of off and the caller is handed the winning value.
| playerSettingsStore.setQuality( | ||
| preset?.resolution ?: resolution, | ||
| preset?.bitrateKbps, | ||
| ) |
There was a problem hiding this comment.
When a legacy tv_prefs file exists but the server has a device-scoped playback.max_bitrate_kbps row without a device-scoped preferred-quality row, this guard passes because the effective lookup neither requests nor checks the bitrate key. setQuality() then overwrites that existing cap with the legacy preset's bitrate—or with JSON null when the legacy quality is absent/default Auto—despite the migration's stated rule that existing server device overrides win. Query and guard both quality axes independently before writing either one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 34767aa. The guard now queries and checks both axes.
Confirmed the mechanism: setQuality writes both rows, but the effective lookup neither requested nor inspected playback.max_bitrate_kbps, so an absent key read as "no override" and the legacy preset's bitrate — or JSON null for a legacy Auto — overwrote an existing server-side cap, contradicting the migration's own stated rule.
Two tests: one asserting a bitrate-only override blocks the quality import, and one asserting the request actually names both keys, since an unqueried axis cannot be guarded no matter what the check says.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
shared/src/commonMain/kotlin/org/siloserver/silo/repository/SettingsRepository.kt (1)
101-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
HttpStatusCode.NotFound.valueinstead of the literal.SettingsApi.getContractCapabilitiesalready uses the typed constant; matching it keeps the status handling consistent.🤖 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/repository/SettingsRepository.kt` around lines 101 - 106, Update clearProfileValue to compare result.code with HttpStatusCode.NotFound.value instead of the literal 404, matching the existing typed status handling used by SettingsApi.getContractCapabilities.shared/src/commonTest/kotlin/org/siloserver/silo/model/settings/SettingsResolve.kt (1)
113-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuedetekt flags this loop; consider a targeted suppression.
The loop is correct (
continuewhen a scope has no row,breakon the first hit), soUnconditionalJumpStatementInLoopis a false positive here. If detekt runs at build-blocking severity, add@Suppress("UnconditionalJumpStatementInLoop")onresolveOnewith a one-line reason rather than restructuring.🤖 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/model/settings/SettingsResolve.kt` around lines 113 - 119, Add a targeted `@Suppress`("UnconditionalJumpStatementInLoop") annotation to resolveOne, including a brief one-line reason that the continue and break statements intentionally implement resolution-order traversal; do not restructure the loop.Source: Linters/SAST tools
androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/testing/FakePlayerSettingsStore.kt (2)
28-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the imported
SubtitleAppearanceinstead of the fully-qualified name.
SubtitleAppearanceis already imported on line 4 and used unqualified on line 48.🤖 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/androidUnitTest/kotlin/org/siloserver/silo/tv/testing/FakePlayerSettingsStore.kt` around lines 28 - 29, Update the effectiveSubtitleAppearanceFlow declaration in FakePlayerSettingsStore to use the already imported SubtitleAppearance symbol instead of its fully qualified name, matching the existing unqualified usage.
139-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilence detekt's
EmptyFunctionBlockwith expression bodies.Detekt flags all four no-op blocks. Expression-body
= Unitis the idiomatic no-op and matches the style already used on line 38.♻️ Proposed change
- override suspend fun refreshFromServer() {} - override suspend fun setSubtitleDeviceOverrideEnabled(enabled: Boolean) {} - override suspend fun resetDeviceSetting(key: String) {} - override suspend fun resetAllDeviceSettings() {} + override suspend fun refreshFromServer() = Unit + override suspend fun setSubtitleDeviceOverrideEnabled(enabled: Boolean) = Unit + override suspend fun resetDeviceSetting(key: String) = Unit + override suspend fun resetAllDeviceSettings() = Unit🤖 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/androidUnitTest/kotlin/org/siloserver/silo/tv/testing/FakePlayerSettingsStore.kt` around lines 139 - 142, Replace the empty block bodies in refreshFromServer, setSubtitleDeviceOverrideEnabled, resetDeviceSetting, and resetAllDeviceSettings with expression bodies returning Unit, matching the existing no-op style in the file.Source: Linters/SAST tools
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStore.kt (1)
445-457: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRead-then-write outside a single
editblock.
snapshotis read beforestore.edit, so a concurrentsetSubtitleAppearancelanding in between is overwritten by the projection computed from the older snapshot. Doing the compare-and-write inside onestore.edit { }(and enqueueing after) removes the window.🤖 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/settings/AndroidPlayerSettingsStore.kt` around lines 445 - 457, The read/compare/write sequence in flushProjectedSubtitleAppearance must be atomic to avoid overwriting concurrent setSubtitleAppearance updates. Move the snapshot lookup, projection/JSON computation, equality check, and preference writes into a single store.edit block, then enqueue the server flush only after a change was written, preserving the existing no-op behavior when the value is unchanged.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.kt (1)
435-444: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe extra flush is redundant after a composite write.
setSubtitleAppearancealready writes the composite (and rewrites the granular slots from it), soflushProjectedSubtitleAppearancehere can only ever project a value equal to what was just stored and no-op. Harmless, but it costs an extra DataStore read per picker edit; the projection flush belongs on paths that write granular slots directly (player HUD).🤖 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/settings/TvSettingsViewModel.kt` around lines 435 - 444, The editAppearance function performs a redundant flush after setSubtitleAppearance already persists the composite and derived granular fields. Remove the flushProjectedSubtitleAppearance call and its explanatory comment from editAppearance, leaving the composite write unchanged; retain projection flushing only in paths that directly modify granular subtitle fields.androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailSubtitlePreferenceTest.kt (2)
163-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer awaiting the
StateFlowover polling on a real dispatcher.
uiState.first { predicate(it) }(wrapped inwithTimeout) removes the 10 ms busy-wait and the hop ontoDispatchers.Default, and fails fast with a clearer signal than a 30 s timeout.🤖 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/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailSubtitlePreferenceTest.kt` around lines 163 - 174, Update the awaitState helper to await viewModel.uiState directly with StateFlow.first { predicate(it) } inside the existing timeout, removing the Dispatchers.Default.limitedParallelism context, polling loop, and delay while preserving predicate-based completion.
187-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEach
FakeSettingsApibuilds a realHttpClientthat is never closed.Three tests × one unused engine; harmless at this scale but it does spin up engine threads. If
SettingsApi's constructor allows it, share a single lazily-created client in the companion object.🤖 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/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailSubtitlePreferenceTest.kt` around lines 187 - 199, Update the FakeSettingsApi test double to reuse a single lazily-created HttpClient from its companion object when calling the SettingsApi constructor, instead of creating a new client per instance. Ensure the shared client remains available across all tests and is closed appropriately if the test lifecycle supports cleanup.android-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/ServerSettingsFlusher.kt (1)
183-191: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
scheduleRetrycancels the coroutine it is running inside.When the retry chain fires,
drainAndFlush()executes insideretryJob; the follow-upscheduleRetry()then callsretryJob?.cancel()while that field still points at the currently executing job. It happens to work today (the replacement job is launched onscope, andwithLockreleases infinally), but it means the retry path routinely self-cancels and any future suspension added after this call indrainAndFlushwould silently abort. Cheap to make explicit.♻️ Skip cancelling our own job
- private fun scheduleRetry(attempt: Int) { + private suspend fun scheduleRetry(attempt: Int) { + val current = currentCoroutineContext()[Job] synchronized(lock) { - retryJob?.cancel() + retryJob?.takeIf { it !== current }?.cancel() retryJob = scope.launch { delay(retryDelayMs(attempt)) drainAndFlush() } } }🤖 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/settings/ServerSettingsFlusher.kt` around lines 183 - 191, Update scheduleRetry so it does not cancel retryJob when that reference is the currently executing coroutine, while continuing to cancel any previous retry job before replacing it. Preserve the existing retry delay and drainAndFlush flow, using the active coroutine context or job identity to distinguish self-cancellation.androidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.kt (1)
399-399: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer named arguments for a 7-dependency ViewModel.
Sibling registrations with this many deps (
PlayerViewModel,DownloadsViewModelaside) use named arguments; a bareget()chain here makes it easy to mis-wire when two constructor parameters share a type and gives no signal at the call site about what was added.🤖 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/di/AndroidModule.kt` at line 399, Update the SettingsViewModel registration to use named arguments for all seven constructor dependencies, matching the parameter names declared by SettingsViewModel; preserve the existing dependency resolution and ordering while replacing the positional get() chain.
🤖 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
`@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStoreTest.kt`:
- Around line 535-543: Replace the nullable check on enqueued in the relevant
AndroidPlayerSettingsStoreTest test with assertNotNull, capture its returned
non-null value, and use that value when decoding SubtitleAppearance so the
subsequent assertions compile unchanged.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileVideoPlaybackStarter.kt`:
- Around line 304-310: Update the subtitle preference assignments in the
playback starter to normalize blank effective and profile language/mode strings
as unset, matching the existing audio-language handling near line 104. Ensure
blank values fall through to the next fallback and ultimately remain null where
no preference is configured, while preserving the existing true default for
showForcedSubtitles.
---
Nitpick comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStore.kt`:
- Around line 445-457: The read/compare/write sequence in
flushProjectedSubtitleAppearance must be atomic to avoid overwriting concurrent
setSubtitleAppearance updates. Move the snapshot lookup, projection/JSON
computation, equality check, and preference writes into a single store.edit
block, then enqueue the server flush only after a change was written, preserving
the existing no-op behavior when the value is unchanged.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/ServerSettingsFlusher.kt`:
- Around line 183-191: Update scheduleRetry so it does not cancel retryJob when
that reference is the currently executing coroutine, while continuing to cancel
any previous retry job before replacing it. Preserve the existing retry delay
and drainAndFlush flow, using the active coroutine context or job identity to
distinguish self-cancellation.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.kt`:
- Line 399: Update the SettingsViewModel registration to use named arguments for
all seven constructor dependencies, matching the parameter names declared by
SettingsViewModel; preserve the existing dependency resolution and ordering
while replacing the positional get() chain.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.kt`:
- Around line 435-444: The editAppearance function performs a redundant flush
after setSubtitleAppearance already persists the composite and derived granular
fields. Remove the flushProjectedSubtitleAppearance call and its explanatory
comment from editAppearance, leaving the composite write unchanged; retain
projection flushing only in paths that directly modify granular subtitle fields.
In
`@androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/testing/FakePlayerSettingsStore.kt`:
- Around line 28-29: Update the effectiveSubtitleAppearanceFlow declaration in
FakePlayerSettingsStore to use the already imported SubtitleAppearance symbol
instead of its fully qualified name, matching the existing unqualified usage.
- Around line 139-142: Replace the empty block bodies in refreshFromServer,
setSubtitleDeviceOverrideEnabled, resetDeviceSetting, and resetAllDeviceSettings
with expression bodies returning Unit, matching the existing no-op style in the
file.
In
`@androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailSubtitlePreferenceTest.kt`:
- Around line 163-174: Update the awaitState helper to await viewModel.uiState
directly with StateFlow.first { predicate(it) } inside the existing timeout,
removing the Dispatchers.Default.limitedParallelism context, polling loop, and
delay while preserving predicate-based completion.
- Around line 187-199: Update the FakeSettingsApi test double to reuse a single
lazily-created HttpClient from its companion object when calling the SettingsApi
constructor, instead of creating a new client per instance. Ensure the shared
client remains available across all tests and is closed appropriately if the
test lifecycle supports cleanup.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/repository/SettingsRepository.kt`:
- Around line 101-106: Update clearProfileValue to compare result.code with
HttpStatusCode.NotFound.value instead of the literal 404, matching the existing
typed status handling used by SettingsApi.getContractCapabilities.
In
`@shared/src/commonTest/kotlin/org/siloserver/silo/model/settings/SettingsResolve.kt`:
- Around line 113-119: Add a targeted
`@Suppress`("UnconditionalJumpStatementInLoop") annotation to resolveOne,
including a brief one-line reason that the continue and break statements
intentionally implement resolution-order traversal; do not restructure the loop.
🪄 Autofix (Beta)
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: 49437d1b-95b1-4aee-9d62-610a3948b50c
📒 Files selected for processing (39)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStore.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/PlayerSettingsStore.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/ServerSettingsFlusher.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStoreTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/ServerDrivenConfigRefresherTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/ServerSettingsFlusherTest.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileVideoPlaybackStarter.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/PlaybackSettings.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/data/preferences/LegacyTvPrefsMigration.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/di/AndroidTvModule.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/data/preferences/LegacyTvPrefsMigrationTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/testing/FakePlayerSettingsStore.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailSubtitlePreferenceTest.ktshared/src/androidUnitTest/kotlin/org/siloserver/silo/model/settings/SettingsConformanceTest.ktshared/src/commonMain/kotlin/org/siloserver/silo/di/RepositoryModule.ktshared/src/commonMain/kotlin/org/siloserver/silo/domain/settings/ProfileSettingsController.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/PlaybackSettingsKeys.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/QualityPresets.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/SettingValueModels.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/SubtitleAppearance.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/SubtitleAppearanceProjection.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/api/SettingsApi.ktshared/src/commonMain/kotlin/org/siloserver/silo/repository/SettingsRepository.ktshared/src/commonTest/kotlin/org/siloserver/silo/domain/settings/ProfileSettingsControllerTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/settings/QualityPresetsTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/settings/SettingsManifest.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/settings/SettingsResolve.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/settings/SubtitleAppearanceProjectionTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/network/api/SettingsApiValuesTest.ktshared/src/commonTest/resources/settings/v1/SOURCEshared/src/commonTest/resources/settings/v1/conformance.jsonshared/src/commonTest/resources/settings/v1/manifest.json
🚧 Files skipped from review as they are similar to previous changes (1)
- androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.kt
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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 552b9fc78c
ℹ️ 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".
| for ((composite, op) in retryable) { | ||
| // A newer op enqueued during the flush wins over the | ||
| // failed one — it is newer content with its own id. | ||
| if (composite !in pending) pending[composite] = op |
There was a problem hiding this comment.
When a write fails transiently and is requeued here, the retained identity contains only (profileId, key), even though the application-scoped flusher later sends relative SettingsApi requests using the currently active server and credentials. Both ServerListViewModel.onSelect and TvServerListViewModel.onSelect can switch servers without clearing this queue, so a retry after that switch targets the new server; a cloned/restored server recognizing the same profile ID can receive the old server's device setting, while other servers leave a stale operation that is revived by later enqueues. Capture the originating server/auth scope with each pending operation or discard it when that scope changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 34767aa. Ops now carry the server they were authored against.
enqueue/enqueueDelete take a serverUrl (the store already had it on Scope), and flushOne drops an op whose origin is no longer the active server rather than sending it. Dropped rather than deferred, deliberately: leaving it queued is what would let a later enqueue revive it against a third server, which was the second half of your finding.
The trade-off is stated in the class doc — persistence is worth a lot, but not worth writing a value to a server the user never authored it against, particularly since a restored or cloned server recognizing the same profile id would accept it rather than fail.
Covered both ways: a retained retry is dropped after a switch and not revived by a later flush, and a transient failure on the unchanged server still retries — otherwise the drop rule would have quietly broken normal persistence.
| return when (val result = repository.getEffectiveValues(PROFILE_KEYS)) { | ||
| is ApiResult.Success -> LoadResult(availability, snapshotOf(result.data)) |
There was a problem hiding this comment.
When any of the three subtitle settings has a supported profile_device override, this effective lookup returns that higher-precedence device value even though every setter below writes only scope=profile. Consequently both settings screens initially label the device override as the profile preference, accept an edit to the shadowed profile row, and keep the optimistic value until a reload reveals that playback still uses the device value. This occurs independently of the policy-constraint case already noted; read the stored profile scope for a pane labeled “Profile,” or expose the winning scope and write/reset it explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 34767aa, together with the constrained-write finding on SettingsViewModel:528 — the two share a root cause, so they share a fix.
You are right that the effective lookup resolves with the device id attached, so a profile_device row outranks the profile row every setter here writes.
I did not switch the read to the stored profile row, though. These panes are not labeled "Profile" in either app (the phone renders a plain "Subtitles" section; TV the same), and what a settings screen should show is what playback will actually do — reading the profile row instead would make the screen honest about the scope while lying about the outcome.
Instead each setter re-resolves after a successful write and returns what the server holds, so a shadowed edit corrects itself immediately rather than at the next reload. Worth noting the shadowing is currently latent on Android: none of the subtitle triple is in PlaybackSettingsKeys.DeviceSettings, so this client never creates such a row itself — it would have to come from another client or an admin write.
If you would rather the pane expose the winning scope and offer an explicit reset, that is a UI decision I would want a product call on rather than fold into a review pass — happy to open a follow-up.
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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34767aa2a1
ℹ️ 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".
| if (fieldOf(snapshot) == edited) { | ||
| // The server agrees with the user's choice — nothing to correct, | ||
| // and rewriting state would clobber a concurrent edit to a | ||
| // different field in the same pane. | ||
| return |
There was a problem hiding this comment.
When a constrained or device-shadowed write is re-resolving while the user successfully edits another profile field, this compares the server snapshot with the first write's authored value rather than checking whether the UI still contains that value. The first response can therefore replace the newer optimistic edit with an older snapshot; when the newer request returns, its snapshot equals its authored value and this early return leaves the UI stale until the screen reloads. Check the current edited field before applying the correction, and avoid overwriting unrelated fields from an earlier request.
Useful? React with 👍 / 👎.
| preferredTextLanguage = watchDetail.effectiveSubtitleLanguage.orNullIfBlank() | ||
| ?: activeProfile?.subtitleLanguage.orNullIfBlank(), |
There was a problem hiding this comment.
When a user clears a previously configured subtitle language, the canonical effective field arrives as the documented empty string, but converting it to null and then using Elvis falls through to the stale user_profiles.subtitle_language column. Since canonical writes are explicitly not mirrored into that column, a profile that previously held en continues preferring English during playback after the user selects no preference. Fall back to the profile column only when the effective field is absent (null), while treating a present blank value as the authoritative null preference; the TV starter contains the same fallback.
Useful? React with 👍 / 👎.
| suspend fun clearProfileValue(key: String): ApiResult<Unit> = | ||
| when (val result = settingsApi.deleteValue(key, SettingScopeIdentity.profile())) { | ||
| is ApiResult.Error -> | ||
| if (result.code == 404) ApiResult.Success(Unit) else result | ||
| else -> result |
There was a problem hiding this comment.
On a server that predates the canonical settings API, deleting a profile preference returns the router's 404 just like any other missing route, and this converts that response into success. Clearing subtitle or metadata language therefore leaves the optimistic blank value in the UI even though the old server retained the previous preference; the subsequent re-resolve fails and cannot correct it. Only treat the canonical endpoint's structured “no row” response as idempotent success, not every HTTP 404.
Useful? React with 👍 / 👎.
* fix(settings): send language tags, not display names
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>
* feat(settings): adopt the generated contract bindings
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>
* feat(settings): add the canonical settings API client
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>
* feat(settings): flush and refresh device settings through the canonical 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>
* feat(settings): write profile and quality preferences at canonical scopes
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>
* test(settings): add the cross-platform conformance runner
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>
* fix(settings): review pass over the canonical API adoption
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>
* fix(settings): address PR #119 review findings
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>
* fix(android): resolve settings end-to-end defects
* fix(android): address review feedback
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Android sends display names where the server's settings contract declares BCP 47 language tags. Found while reviewing the server-side settings contract (Silo-Server/silo-server#479), which starts validating these.
The bug
PlaybackSettings.kt's picker is a list of English words, andSettingsDropdownRowhands the selected string straight to the callback. The only mapping on the path is"Default" <-> "", so everything else reaches the wire verbatim:The phone's subtitle picker does the same to the profile's
subtitle_language. The TV does it for audio while doing it correctly for subtitles, with a comment recording the label-as-id choice as intentional.metadataLanguageOptions, one screen over in the same file, already had the correct pattern.This is not new breakage. The same string is read back and handed to ExoPlayer:
setPreferredAudioLanguage("English")never matches a track taggedeng, so picking an audio language on Android has silently done nothing. It also means Android and Apple write different vocabularies to the same key — Apple has always sent codes, so a language set on an iPhone reads as "Default" here, and one set here does not match Apple's picker either.What is new: the server now validates the tag, so the PUT 400s.
ServerSettingsFlusheronly logs a failure, so after a server upgrade the setting stops persisting entirely, with no user-visible error.The fix
One
LanguageOptionstable inshared, replacing five lists that had already drifted into two different conventions. Every picker renderslabel(wire)and emitswireValue(label), so a language cannot be added to one surface and missed on the others.Values already stored on devices are translated on read (
migrateLegacyValue), so an existing"English"becomes"en"rather than being re-sent to a server that will reject it or handed to a track selector that will ignore it. An unrecognized value clears instead of failing validation on every flush.The ViewModel state fields now hold wire values rather than labels; the pickers do the conversion, which is where the other settings in these screens already do it.
Verification
LanguageOptionsTestpins that every option persists a tag rather than its label, that labels and wire values round-trip, that an unknown or legacy value reads as unset rather than being echoed back as a choice the server does not hold, and that the legacy labels older builds actually wrote migrate to their tags.Also: generated contract bindings
SettingKeys.ktis now generated from the server's manifest bycmd/settingsgen, and the two hand-maintained tables inAndroidPlayerSettingsStoredelegate to it.BOOLEAN_KEYS/INT_KEYS/DOUBLE_KEYSwas a second table that had to agree withPlaybackSettingsKeys.DeviceSettingsby discipline alone — a key added to one and missed in the other flushes as the wrong type and is silently dropped on read. Both are manifest questions now.A new contract test (
SettingKeysContractTest) immediately caught two real drifts: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 shippedplayer.*while Apple and the server usedplayback.*, so the same preference was two settings and neither client could read the other's.It also caught a generator bug on the server side:
client_localkeys were being classified by type, describing a wire format for values that never cross the wire.Now also: the canonical settings API adoption
The commits after
abf6334acomplete this client's half of the cutover (821b02b2..112489a1): Android now calls the canonical API rather than just sharing its key names.API client (
SettingsApi):getContractCapabilities()(a typedServerUpgradeRequiredon a bare 404 — an enveloped 404 such as a deleted profile is not misread as an old server), batchedgetEffectiveValues(keys, libraryIds, seriesIds), andputValue/deleteValuewithX-Silo-Mutation-Ididempotency. Wire models mirrorsettings_values.gofield-for-field; profile/device identity rides the session headers the auth interceptor already attaches.Flusher and store:
ServerSettingsFlusherspeaksPUT/DELETE /settings/values/{key}?scope=profile_devicewith contract-typed JSON (empty language tag → JSONnull, since the server rejects""), keeps the 750 ms debounce, and a transiently-failed write now stays queued and retries with the same mutation id instead of vanishing intoLog.w— the silent-loss failure mode this PR's original bug report described is closed, not just relocated.refreshFromServer()is one batched effective call;source=="default"entries hydrate generated contract defaults, absent keys keep local values.Profile prefs and quality: the profile subtitle triple plus metadata language moved off
UpdateProfileRequestontoscope=profilewrites, phone and TV routed through one sharedProfileSettingsControllerso the screens cannot drift. Quality is the two-axis preset table shared with web (playback.preferred_quality+playback.max_bitrate_kbps); the granularsubtitle.*fields stay client-local and project into the compositeplayback.subtitle_appearanceon flush. A server without the canonical routes gets an explanatory notice instead of an empty screen. One deliberate divergence to review: quality is written at device scope — the Android store already owns both axes asprofile_devicerows, and a profile-scope write would be permanently shadowed by this device's own row.Conformance runner: the server's fixture and manifest vendored byte-identical (provenance in
SOURCE), with a Kotlin resolver running all 24 cases in:shared:testDebugUnitTest; unknown fixture fields and manifest-revision mismatch are failures.Review pass (
112489a1): four adversarial lenses, 8 findings, 6 confirmed by runnable probes, each fix pinned by a test verified failing-first (six separate revert runs). The two most consequential: a re-queued failed write could revert a newer value for the same key sent in the same flush (now drop-if-superseded), and phone playback plus the TV detail preview still read the legacy profile columns the settings screen stopped writing (both now route through the controller — including an""-vs-nulltranslation that would otherwise have disabled the subtitle Auto preview for anyone with no language set). The TV legacy quality import now maps through the preset table so the bitrate axis survives.Verification at
112489a1::shared:testDebugUnitTestand:android-shared:testDebugUnitTestfully green (rerun with--rerun-tasksto defeat stale caches), both app targets compile, working tree clean.Follow-ups (not this PR): the store still clamps playback speed to 4.0 where the contract says 3.0 (server rejects >3.0; UI presets stop at 3.0, so edge-case only), and the conformance fixture's empty-device-id guard is unpinned in all four runners — the case belongs upstream in
contracts/settings/v1/conformance.json.Merge order
This should land before Silo-Server/silo-server#479, or Android users lose audio- and subtitle-language selection for one release.
AI Disclosure
claude-opus-5[1m],claude-fable-5SettingsViewModel.audioLanguageWireValue,AndroidPlayerSettingsStore.writeString,ServerSettingsFlusher.enqueue,SettingsApi.setDeviceSetting— rather than inferred from the picker alone, and the same trace on silo-apple confirmed Apple sends codes, which is what established this as an Android-side defect rather than a contract that was too strict. The alternative considered and rejected was widening the server to accept display names: it would heal stored values without a client release, but it permanently admits non-BCP-47 aliases into the contract and leaves the ExoPlayer no-op unfixed. The on-device migration was added after noting that fixing the pickers alone would leave existing installs re-sending a rejected value indefinitely.Summary by CodeRabbit