fix(settings): realign device settings with the server contract - #101
fix(settings): realign device settings with the server contract#101Quick104 wants to merge 3 commits into
Conversation
Android's copy of the settings contract had drifted from the server
registry in internal/api/handlers/settings.go, so several keys were
written with values or names the server does not accept.
- Rename PlaybackSettingsKeys.NextUpPromptSeconds from
"player.next_up_prompt_seconds" to the canonical
"playback.next_up_prompt_seconds" that the server and Apple use.
Note: current server main aliases the old Android spelling to the
canonical key (canonicalDeviceSettingKey, added 2026-07-23), so this
is not active data loss today — it drops Android's dependence on a
deprecated compatibility shim. Servers older than that alias never
registered the "player." spelling at all, so moving to the canonical
key is compatible in both directions.
nextUpPromptSecondsFlow keeps a read-only fallback to the old
DataStore slot so existing installs do not silently reset to 30.
- Clamp playback speed to 0.25..3.0 to match
validateFloatRange("player.playback_speed", 0.25, 3.0). No UI can
request more than 3.0 today, so this is a contract fix with no
user-visible change.
- Default player.dv_profile7_hdr10_fallback to false before hydration,
matching the server registry default and Apple. The phone and TV
settings UI state defaulted to true as well, which rendered the
toggle ON until the store flow emitted; both are flipped. This is a
real behavior change for users who never touched the toggle and are
offline or have no stored override.
- Drop player.match_frame_rate, player.sleep_timer_default_minutes and
the nine individual subtitle.* appearance fields from DeviceSettings.
The server does not register them, so every write and every reset
returned HTTP 400. They keep working locally: the two live settings
now write through writeBoolLocal/writeIntLocal, so their flows,
clamps and defaults are unchanged. The subtitle.* constants are dead
code (nothing reads or writes them); Android's subtitle styling all
flows through the single subtitle_appearance JSON blob the server
registers, so there is no blob-vs-fields consistency to maintain.
Unifying or deleting them is follow-up work.
Also removes the four now-unreachable entries from the BOOLEAN_KEYS /
INT_KEYS type-dispatch sets, which only ever see DeviceSettings keys.
Part of #376
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eading it The key rename in the previous commit shipped a read-only fallback to the old "player.next_up_prompt_seconds" DataStore slot, and both the code comment and the commit message claimed that a later write or server refresh would migrate it forward. Neither does, and a refresh actively destroys it. applyEffectiveLocally writes the server's effective value into the canonical slot for every registered DeviceSettings key. Before the rename that could not clobber a local value: the "player." spelling was unregistered server-side, so resolveEffectiveSetting returned an empty effective value and writeRawString's Int branch skipped the write. The canonical key IS registered, with a registry default of 30, so after the rename the first refresh writes 30 into the canonical slot, which then shadows the legacy slot permanently. refreshFromServer runs on settings-screen open, player load, TV detail, and ServerDrivenConfigRefresher, so that happens within seconds of upgrading. The affected users are precisely those whose value never reached the server: an install talking to a pre-alias server, or one whose flush failed silently, which ServerSettingsFlusher only logs. They would have seen their next-up prompt silently reset to 30. Fix: migrateNextUpPromptKey copies the legacy slot into the canonical slot once per scope, removes the legacy slot, and enqueues the migrated value so the server stops reporting the registry default. It runs at the top of ensureMigrated, which every read path (profileScopedFlow) and every write path (withScope, and therefore refreshFromServer) funnels through, so it is guaranteed to happen before anything can overwrite it. It carries its own sentinel because existing installs have already recorded the ensureMigrated one, and folding it in there would skip exactly the users who need it. The read-time fallback is removed. It is dead once the value is copied forward, and keeping it would let a stale value resurface if the canonical slot were later cleared. Also handles the same stranding in the legacy-cache import: ensureMigrated looked up AndroidServerSettingsCache by the new key name, which an old install would have stored under the old one. Suppressing the refresh write instead - skipping keys the server reports without a device override - was considered and rejected. The subtitle-appearance block in applyEffectiveLocally deliberately propagates a server-side reset, and a blanket guard would break that for every key. The general "a failed flush lets the next refresh overwrite a local value" problem is real but belongs with the durable outbox work, not here. Tests: three cases covering forward-copy plus push, not clobbering an existing canonical value, and not re-running. Verified they catch the regression by disabling migrateNextUpPromptKey and re-running - 3 failed, 26 pass with it. Also reverts the docs/superpowers/plans/ edit from the previous commit; plan files are historical records and should not be annotated by later work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughPlayback setting contracts, migrations, server synchronization, local-only writes, defaults, reset validation, and Android test coverage are updated to align local behavior with server-accepted settings. ChangesPlayback settings synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Quick104
changed the title
…verwrites it The next-up rename migration copied the value forward and enqueued it for the server, but the enqueue is debounced by 750 ms and withScope runs the migration and then falls straight into refreshFromServer. On a LAN server the GET returns in ~30 ms with the registry default of 30, applyEffectiveLocally writes it over the value just recovered, and the sentinel is already committed so the migration never runs again. The value is lost for exactly the users the migration exists to protect, and its own KDoc says the push is what protects it. refreshFromServer now flushes pending writes before it reads, which is what setSubtitleDeviceOverrideEnabled already did. This also fixes the general case: any local write that has not yet flushed used to be reset by the next refresh. When the flush fails the GET almost always fails with it, and the existing early return leaves local values untouched. The legacy-SharedPreferences import had the same hole and no push at all — it wrote recovered values into local slots that the next refresh would overwrite with registry defaults. It now enqueues what it imports. Values are clamped to the server's range before being sent, so an out-of-band legacy value is stored rather than 400'd and silently dropped, and the two range constants are shared with the setters instead of repeated as literals. The KDoc's stated reason for not fixing the root cause was wrong: it claimed a hasDeviceOverride gate on the apply loop would break the subtitle-appearance reset, but that block sits outside the loop and reads hasDeviceOverride itself. The real objection is resetAllDeviceSettings, which deletes every device row and refreshes precisely to pull the defaults back down. Corrected in place. resetDeviceSetting is the only path that can reach the server with a caller-supplied key and had no membership check; a device-local key would have produced a silent 400 behind an apparently successful round trip. Tests: the two new ordering tests fail without the flush (expected 15, got 30). Adds coverage for the scoped key path, which every existing migration test missed by running with a null deviceId — in production getDeviceId always resolves, so the scoped prefix is the only path that ships. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![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: 71dd26008b
ℹ️ 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".
| imported.forEach { (key, value) -> | ||
| serverSettingsFlusher.enqueue(scope.profileId, key, clampForServer(key, value)) |
There was a problem hiding this comment.
When an upgrade has legacy cache values but no DataStore migration sentinel, this now pushes every cached value before fetching effective settings, so a stale cache overwrites a newer device override already stored on the server. The TV migration explicitly checks hasDeviceOverride before importing for this reason (LegacyTvPrefsMigration.kt:109-136); this migration needs the same server-authority guard rather than unconditionally enqueueing the cache.
Useful? React with 👍 / 👎.
| // | ||
| // If the flush fails the GET almost always fails with it, and the | ||
| // early return below leaves the local value untouched. | ||
| serverSettingsFlusher.flushNow() |
There was a problem hiding this comment.
If a migrated PUT fails but the immediately following GET succeeds, this still applies the server's stale/default value over the recovered local value. DefaultServerSettingsFlusher.flushNow() returns Unit and merely logs and drops API/network failures (ServerSettingsFlusher.kt:87-102), so success cannot be inferred here; for the next-up migration the legacy slot and sentinel have already been committed, making that overwrite permanent and preventing a retry.
Useful? React with 👍 / 👎.
| if (store.data.first()[sentinelKey] != true) { | ||
| val canonicalKey = | ||
| intPreferencesKey(scope.keyPrefix + PlaybackSettingsKeys.NextUpPromptSeconds) | ||
| val legacyKey = intPreferencesKey(scope.keyPrefix + LEGACY_NEXT_UP_PROMPT_SECONDS) |
There was a problem hiding this comment.
When an older install wrote this setting while deviceId or serverUrl was unavailable, it lives in the unscoped player.next_up_prompt_seconds slot; after scope information becomes available, existing reads deliberately fall back to unscoped keys (scopedRead, lines 663-676). This migration searches only scope.keyPrefix + LEGACY_NEXT_UP_PROMPT_SECONDS, so a currently scoped upgrade never copies that valid unscoped value and the renamed flow falls back to 30 instead. Check the unscoped legacy slot as a fallback when the scoped legacy slot is absent.
Useful? React with 👍 / 👎.
| DvProfile7HDR10Fallback, | ||
| DolbyVisionEnabled, | ||
| MatchContentFrameRate, | ||
| SleepTimerDefaultMinutes, | ||
| SubtitleFontSize, | ||
| SubtitleFontFamily, | ||
| SubtitleTextColor, | ||
| SubtitleBackgroundColor, | ||
| SubtitleBackgroundStyle, | ||
| SubtitleBackgroundOpacity, | ||
| SubtitleTextOutline, | ||
| SubtitleTextOutlineColor, | ||
| SubtitlePosition, | ||
| ) |
There was a problem hiding this comment.
For an upgrade whose DataStore migration sentinel has not yet been written, ensureMigrated imports only keys in DeviceSettings; ending this list here therefore stops importing legacy MatchContentFrameRate and SleepTimerDefaultMinutes values. Those settings are still read from the same local DataStore keys and are meant to remain functional locally, so users migrating from the SharedPreferences cache silently revert to false and 30 even though the setter changes are intended only to stop server synchronization. Import these two keys through a separate local-only migration list.
Useful? React with 👍 / 👎.
| if (legacyValue != null && prefs[canonicalKey] == null) { | ||
| prefs[canonicalKey] = legacyValue | ||
| migratedValue = legacyValue |
There was a problem hiding this comment.
When the legacy DataStore slot contains an out-of-range value, this writes that raw value into the canonical local slot while only the enqueued server value is clamped. If the app is offline, has no settings repository, or simply reads the flow before a successful refresh, nextUpPromptSecondsFlow can therefore expose values outside the setter's supported 0–120 range indefinitely; the new 300-value test checks only the flusher call and misses this local mismatch. Clamp once before assigning prefs[canonicalKey] and enqueue the same value.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStoreTest.kt (1)
321-332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
assertFailsWithinstead of the manual try/catch flag.♻️ Proposed simplification
fun `resetDeviceSetting refuses a key that is not server-synced`() = runTest { val store = newStore(repository = SettingsRepository(FakeSettingsApi())) - var threw = false - try { - store.resetDeviceSetting(PlaybackSettingsKeys.PictureInPictureEnabled) - } catch (_: IllegalArgumentException) { - threw = true - } - assertTrue(threw, "A device-local key must not reach DELETE /settings/device/{key}.") + assertFailsWith<IllegalArgumentException>( + "A device-local key must not reach DELETE /settings/device/{key}.", + ) { + store.resetDeviceSetting(PlaybackSettingsKeys.PictureInPictureEnabled) + } assertTrue(fakeFlusher.calls.isEmpty(), "Nothing should have been queued for the server.") }Requires
import kotlin.test.assertFailsWith.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStoreTest.kt` around lines 321 - 332, Update the test method resetDeviceSetting refuses a key that is not server-synced to use kotlin.test.assertFailsWith<IllegalArgumentException> around the resetDeviceSetting call, replacing the manual threw flag and try/catch while preserving the existing fakeFlusher.calls assertion.android-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStore.kt (1)
118-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
clampForServeronly guards two of the fifteenDeviceSettingskeys.Legacy-cache import now enqueues every imported key to the server (a new behavior), but
clampForServeronly clampsNextUpPromptSecondsandPlaybackSpeed; everything else (e.g.VideoGravity,OrientationMode) is pushed raw. Currently safe only because legacy-cached values were previously server-accepted, but it's a silent gap if a futureDeviceSettingsentry gets a server-side range/enum constraint without a matchingclampForServerbranch — the push would 400 and get dropped by the flusher with no visible signal.Consider a small comment or a
whenexhaustiveness note tyingclampForServeradditions toDeviceSettingschanges, so future key additions don't miss this.Also applies to: 150-163
🤖 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 118 - 148, Document the coupling between PlaybackSettingsKeys.DeviceSettings and clampForServer: add a concise maintenance comment or exhaustiveness note at clampForServer stating that every DeviceSettings key requiring server-side range or enum validation must have a corresponding branch when new keys are added. Keep the existing clamping behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStore.kt`:
- Around line 118-148: Document the coupling between
PlaybackSettingsKeys.DeviceSettings and clampForServer: add a concise
maintenance comment or exhaustiveness note at clampForServer stating that every
DeviceSettings key requiring server-side range or enum validation must have a
corresponding branch when new keys are added. Keep the existing clamping
behavior unchanged.
In
`@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStoreTest.kt`:
- Around line 321-332: Update the test method resetDeviceSetting refuses a key
that is not server-synced to use
kotlin.test.assertFailsWith<IllegalArgumentException> around the
resetDeviceSetting call, replacing the manual threw flag and try/catch while
preserving the existing fakeFlusher.calls assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93e70442-af63-4f5c-98ce-a5808bda5038
📒 Files selected for processing (5)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStore.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStoreTest.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/PlaybackSettingsKeys.kt
|
Closing this PR as superseded by #119 and the current settings implementation in #146. The branch is no longer suitable to merge as-is because the generated contract bindings and canonical settings API changed the relevant architecture. The remaining actionable findings have been split into focused issues:
The old manual contract-table changes, and the changes that made match-frame-rate and sleep-timer local-only, should not be carried forward. |
1 participant
Android's copy of the settings contract had drifted from the server registry in
internal/api/handlers/settings.go. Verified findings from Silo-Server/silo-server#376.What changed
PlaybackSettingsKeys.ktNextUpPromptSeconds:player.next_up_prompt_seconds→playback.next_up_prompt_seconds, matching the server and AppleDeviceSettings):player.match_frame_rate,player.sleep_timer_default_minutes, and ninesubtitle.*fields. The server registers none of them, sokeyUsesDeviceScopereturns false and both the set and delete handlers 400 before validation — every write and every reset for these was being rejectedAndroidPlayerSettingsStore.kt4.0→3.0, matchingvalidateFloatRange("player.playback_speed", 0.25, 3.0)player.dv_profile7_hdr10_fallbackdefaulttrue→false, matching the server and ApplesetMatchContentFrameRateandsetSleepTimerDefaultMinutesmove to the*Localhelpers — identical preference keys, identical clamps, read flows untouched, so local behavior is preserved exactly; they simply stop syncingmigrateNextUpPromptKey(see below)Tests — the speed clamp assertion was updated, not weakened: it now asserts both bounds (
10.0 → 3.0,0.01 → 0.25). Three new migration cases.The migration bug, found in review
The first commit shipped a read-only fallback to the old DataStore slot and claimed a later write or refresh would migrate it forward. Neither does, and a refresh actively destroys it.
applyEffectiveLocallywrites the server's effective value into the canonical slot for every registered key. Before the rename that could not clobber anything — theplayer.spelling was unregistered, so the server returned an empty value and the Int branch skipped the write. The canonical key is registered, with a registry default of30, so the first refresh after upgrade writes 30 and it shadows the legacy slot permanently.refreshFromServerruns on settings-screen open, player load, TV detail, andServerDrivenConfigRefresher— within seconds of upgrading. Affected users are exactly those whose value never reached the server: a pre-alias server, or a flush that failed silently.migrateNextUpPromptKeycopies the legacy slot forward once per scope, removes it, and enqueues the migrated value so the server stops reporting the registry default. Copying locally is not enough on its own — the refresh would still overwrite it. It runs at the top ofensureMigrated, which every read path and every write path funnels through, and carries its own sentinel because existing installs have already recorded theensureMigratedone; folding it in there would skip precisely the users who need it.Suppressing the refresh write instead (skipping keys the server reports without a device override) was considered and rejected: the subtitle-appearance block deliberately propagates a server-side reset, and a blanket guard would break that for every key.
Second review pass — the migration's push was still losing the race
The migration commit above enqueued the recovered value for the server, and its own KDoc said that push is what protects it. It does not, on its own:
enqueueis debounced 750 ms, andwithScoperunsensureMigratedand then falls straight intorefreshFromServer. On a LAN server the GET returns in ~30 ms with the registry default of 30,applyEffectiveLocallywrites it over the value just recovered, and the sentinel is already committed so the migration never runs again. Deterministic, not a rare race — and it loses the value for exactly the users the migration exists to protect.refreshFromServernow flushes pending writes before it reads, which is whatsetSubtitleDeviceOverrideEnabledalready did ten lines away. This also closes the general case: any local write that had not yet flushed was reset by the next refresh. When the flush fails the GET almost always fails with it, and the existing early return leaves local values untouched.The legacy-SharedPreferences import had the same hole and no push at all — it wrote recovered values into local slots that the next refresh would overwrite with registry defaults. It now enqueues what it imports, clamped to the server's accepted range so an out-of-band legacy value is stored rather than 400'd and silently dropped.
The KDoc's stated reason for not fixing the root cause was wrong. It claimed a
hasDeviceOverridegate on the apply loop would break the subtitle-appearance reset. That block sits outside the loop and readshasDeviceOverrideitself, so gating the loop would not touch it. The real objection isresetAllDeviceSettings, which deletes every device row and refreshes precisely to pull the defaults back down — a gate would make reset a no-op. Corrected in place so the next reader isn't misled.resetDeviceSetting(key)— listed below as a follow-up — is fixed here too, since it is the only path that can reach the server with a caller-supplied key.Test coverage. Every existing migration test ran with
deviceId = null, which makeskeyPrefixempty. In productiongetDeviceIdalways resolves, so the scoped prefix is the only path that ships and it had zero coverage. Added. The two new ordering tests were confirmed to fail without the fix, with the exact symptom:expected:<15> but was:<30>.Correcting the issue's framing
Current server
mainalready aliases the old Android spelling viacanonicalDeviceSettingKey, with legacy-fallback reads and dual delete. So this was not active data loss against a current server — the rename drops Android's dependence on a deprecated compatibility shim so that shim can eventually be retired. #376's "never reaches the server" was only true against pre-alias servers.Verification
Negative control — with the
flushNow()removed fromrefreshFromServer, two tests failand nothing else does:
Out of scope, worth follow-ups
player.orientation_modeas{landscapeLocked, rotateFreely}, butorientationModeFlowdefaults to"auto"andsetOrientationModewrites unvalidated — any value outside those two 400sresetDeviceSetting(key)forwards an arbitrary key toenqueueDeletewith no membership check.DeviceSettings.match_frame_rateandsleep_timer_default_minutesare registered in the contract manifest (feat(settings): add the cross-platform settings contract and its manifest silo-server#479), so they return to the sync list at that cutover. Temporary churn, but they are rejected todaysubtitle.*names had zero readers and zero writers; all Android subtitle styling already goes through thesubtitle_appearanceblob, which the server does register. Unifying them is follow-up workPart of Silo-Server/silo-server#376. Server-side contract: Silo-Server/silo-server#479.
AI Disclosure
claude-opus-5[1m]applyEffectiveLocally's loop onhasDeviceOverride— correct that the KDoc's stated objection was bogus, wrong that the gate is safe, because it would silently breakresetAllDeviceSettings.Test results were re-run independently rather than taken from the implementing agent's report.
Summary by CodeRabbit
Improvements
Bug Fixes