From 1a9a6084c0d873a31324c98f3555da14e5154903 Mon Sep 17 00:00:00 2001 From: Quick104 <31828688+Quick104@users.noreply.github.com> Date: Sun, 16 Aug 2026 21:13:56 -0400 Subject: [PATCH] fix(settings): keep the intro-skip profile column truthful MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #660 found the legacy user_profiles.auto_skip_intro column could be moved by an enum write and never moved back, and a new three-way control landing on the web device screen beside the switch it replaces. - The column now tracks every canonical profile-scope change to either half of the pair. Previously only playback.intro_skip_mode wrote it, so an "always" write pinned it true: a following auto_skip_intro=false stored false in the rows while GET /profiles kept saying true, and a DELETE that went back to inheriting left the column asserting a choice nobody held — which the web player reads straight out of the profile DTO and acts on. - Clearing a profile-scope row now falls the column back to the contract default, read from the manifest rather than spelled in the handler. - playback.intro_skip_mode is hidden from the web device settings screen for this release. It had no group entry, so it fell through to its manifest category and rendered under Picture while "Auto-skip intros" stayed under Episodes: two controls for one preference, each silently rewriting the other through the mirror, and a "never" the player cannot honour yet. Its group is recorded so unhiding it in the player follow-up is one deletion. Co-Authored-By: Claude Opus 5 --- docs/design/2026-08-16-intro-skip-mode.md | 12 +- docs/settings-api.md | 2 +- internal/api/handlers/settings_values.go | 108 ++++++++++++++---- .../handlers/settings_values_mirror_test.go | 91 +++++++++++++++ web/src/lib/deviceSettingGroups.test.ts | 10 ++ web/src/lib/deviceSettingGroups.ts | 14 +++ 6 files changed, 210 insertions(+), 27 deletions(-) diff --git a/docs/design/2026-08-16-intro-skip-mode.md b/docs/design/2026-08-16-intro-skip-mode.md index 7b6c7188..8ba7c5ec 100644 --- a/docs/design/2026-08-16-intro-skip-mode.md +++ b/docs/design/2026-08-16-intro-skip-mode.md @@ -77,7 +77,15 @@ time so a preference set on one client shows up correctly on the others: | `DELETE` of either | delete the other at the same identity | | Legacy `PUT /profiles/{id}` with `auto_skip_intro` | both keys at `profile` scope (extends `profiles_settings_sync`) | | Legacy `PUT /settings/{key}` (runtime-key route) with `auto_skip_intro` | both keys, wherever that route lands them | -| `intro_skip_mode` written at `profile` scope | the `user_profiles.auto_skip_intro` column, so `GET /profiles` stays truthful for old clients | +| Either key written or cleared at `profile` scope | the `user_profiles.auto_skip_intro` column, so `GET /profiles` stays truthful for old clients | + +The column is the third copy of one preference, and it tracks every canonical +profile-scope change to either key — not only the enum. A column only the enum +could move would end up contradicting the row the caller stored: an enum write +sets it, and the boolean write or the `DELETE` that follows could not put it +back. A cleared row means "inherit again", so the column falls back to the +contract default with it. Device-scope writes leave it alone; it is +profile-wide, and one television's override is not the household's choice. The lossy direction is deliberate: an old client sees `never` as `false` (= ask) and shows the button. That is the least surprising degradation. An old @@ -197,7 +205,7 @@ Never / Ask to skip / Skip automatically; the old switch goes away. | Repo | Work | | --- | --- | -| `silo-server` (this change) | Manifest rev 7, migration, write mirror, bindings regen, web settings control reads/writes the enum. Web player behaviour to the tables above is a follow-up in the same repo. | +| `silo-server` (this change) | Manifest rev 7, migration, write mirror, bindings regen. Web keeps showing only the `auto_skip_intro` switch: two controls for one preference would disagree the moment either was touched, and the player cannot honour `never` yet. The web control and the player behaviour above land together in a follow-up in the same repo. | | `silo-android` | `IntroAutoSkipController` (shared KMP, drives phone + TV) gains the three modes and the `Skipped/OfferingUndo` state; `TvIntroAutoSkipBanner` / `IntroAutoSkipBanner` render both copies; settings screens swap the switch for a segmented control; `SettingKeys.kt` regenerated by `make settings-bindings`. | | `silo-apple` | Same state machine in the shared player; tvOS focus rules; iOS/macOS pointer rules; `SettingKeys.generated.swift` regenerated. | diff --git a/docs/settings-api.md b/docs/settings-api.md index 7ac22465..7d8a6e9e 100644 --- a/docs/settings-api.md +++ b/docs/settings-api.md @@ -153,7 +153,7 @@ client shows up correctly on the others: | `DELETE` of either | removes the other at the same identity | | `PUT`/`POST /profiles` with `auto_skip_intro` | writes both keys at `profile` scope | | `PUT`/`DELETE` of the legacy `/settings/{key}` or `/settings/device/{key}` for `playback.auto_skip_intro` | writes or clears both keys at the scope that route owns | -| `playback.intro_skip_mode` at `profile` scope | updates `user_profiles.auto_skip_intro`, so the profile DTO stays truthful for clients that read it | +| `PUT` or `DELETE` of either key at `profile` scope | sets `user_profiles.auto_skip_intro` to what the pair now resolves to — the written value, or the contract default once the row is cleared — so the profile DTO stays truthful for clients that read it | Both rows commit together on the idempotent write path, and a replayed mutation id re-serves its receipt without writing either again. The response is diff --git a/internal/api/handlers/settings_values.go b/internal/api/handlers/settings_values.go index bedfc5d5..e00f4f87 100644 --- a/internal/api/handlers/settings_values.go +++ b/internal/api/handlers/settings_values.go @@ -889,9 +889,10 @@ func (h *SettingValuesHandler) setValueAt( } // GET /profiles still serves auto_skip_intro from the legacy column, so a - // profile-scope choice made through the new key has to reach it or the - // profile DTO keeps reporting the preference the household abandoned. - if !h.syncLegacyIntroSkipColumn(r, w, store, identity, mirror, hasMirror) { + // profile-scope choice made through either half of the pair has to reach it + // or the profile DTO keeps reporting the preference the household abandoned. + if !h.syncLegacyIntroSkipColumn(r, w, store, identity, + legacyIntroSkipColumnWrite(identity.Key, normalized, mirror, hasMirror)) { return } @@ -931,46 +932,98 @@ func (h *SettingValuesHandler) setValueAt( } } -// syncLegacyIntroSkipColumn mirrors a profile-scope playback.intro_skip_mode -// write into user_profiles.auto_skip_intro, and reports whether the request may -// continue. +// legacyIntroSkipColumnWrite is the playback.auto_skip_intro value a canonical +// write implies, or nil when the write is not part of the intro-skip pair. // -// It is deliberately one-directional. The canonical write path does not -// otherwise touch the legacy preference columns — the cutover direction is that -// the profile DTO stops reading them, which it already has for the language and -// subtitle fields — so this is not a general dual-write, only the narrow repair -// that keeps the one DTO field still served from its column truthful while the -// key that replaced it is the one clients write. A write of the deprecated -// boolean itself is left alone, exactly as it is today. +// Either half answers, because the mirror has already made them one preference: +// the boolean is its own answer, and the enum's is the companion just computed +// for it. Both have to answer, or the column would be movable in one direction +// only — an enum write could set it and the boolean write that followed could +// not correct it. +func legacyIntroSkipColumnWrite( + key string, + normalized json.RawMessage, + mirror settingscontract.MirroredWrite, + hasMirror bool, +) json.RawMessage { + if !hasMirror { + return nil + } + switch key { + case settingskeys.PlaybackAutoSkipIntro: + return normalized + case settingskeys.PlaybackIntroSkipMode: + return mirror.Value + default: + return nil + } +} + +// legacyIntroSkipColumnCleared is the playback.auto_skip_intro value a cleared +// profile-scope row leaves behind, or nil when the cleared key is not part of +// the intro-skip pair. +// +// Clearing the profile-scope row is how a household says "inherit again", so +// what the column must now hold is what the key resolves to with no explicit +// value: the contract default. It is read from the manifest rather than spelled +// here so a changed default cannot leave the column behind. +func (h *SettingValuesHandler) legacyIntroSkipColumnCleared(key string) json.RawMessage { + switch key { + case settingskeys.PlaybackAutoSkipIntro, settingskeys.PlaybackIntroSkipMode: + default: + return nil + } + def, ok := h.contract.Lookup(settingskeys.PlaybackAutoSkipIntro) + if !ok || len(def.DefaultValue) == 0 { + return nil + } + return def.DefaultValue +} + +// syncLegacyIntroSkipColumn writes the intro-skip preference through to +// user_profiles.auto_skip_intro, and reports whether the request may continue. +// A nil value means this request has nothing to say about the column. +// +// GET /profiles still serves auto_skip_intro from its column — the profile +// DTO's shape is pinned by shipped clients — so the column is a third copy of +// one preference and has to track every canonical profile-scope change to +// either half of the pair, set or clear alike. A column that only the enum +// write could move would end up contradicting the very row the caller stored: +// an enum write sets it true, and a later boolean write, or a DELETE that goes +// back to inheriting, leaves the DTO reporting a choice nobody holds. +// +// Device scope is none of its business: the column is profile-wide, and one +// television's override is not the household's choice. The canonical write path +// touches no other legacy preference column — the cutover direction is that the +// profile DTO stops reading them, which it already has for the language and +// subtitle fields — so this stays the narrow repair for the one field still +// served from its column. // // A failure fails the request for the same reason the companion row write does: // the caller asked for one preference change, and reporting success while half -// of it landed is what makes the two halves drift. +// of it landed is what makes the copies drift. func (h *SettingValuesHandler) syncLegacyIntroSkipColumn( r *http.Request, w http.ResponseWriter, store userstore.UserStore, identity userstore.SettingIdentity, - mirror settingscontract.MirroredWrite, - hasMirror bool, + value json.RawMessage, ) bool { - if !hasMirror || - identity.Key != settingskeys.PlaybackIntroSkipMode || - identity.Scope != settingscontract.ScopeProfile { + if value == nil || identity.Scope != settingscontract.ScopeProfile { return true } var enabled bool - if err := json.Unmarshal(mirror.Value, &enabled); err != nil { - slog.ErrorContext(r.Context(), "settings mirror produced a non-boolean auto_skip_intro", - "component", "api", "profile_id", identity.ProfileID, "error", err) + if err := json.Unmarshal(value, &enabled); err != nil { + slog.ErrorContext(r.Context(), "intro skip preference produced a non-boolean auto_skip_intro", + "component", "api", "profile_id", identity.ProfileID, "key", identity.Key, "error", err) writeError(w, http.StatusInternalServerError, "internal_error", "Failed to store the setting") return false } if err := store.UpdateProfile(r.Context(), identity.ProfileID, userstore.UpdateProfileInput{AutoSkipIntro: &enabled}); err != nil { - slog.ErrorContext(r.Context(), "failed to mirror intro skip mode into the profile column", - "component", "api", "profile_id", identity.ProfileID, "error", err) + slog.ErrorContext(r.Context(), "failed to mirror the intro skip preference into the profile column", + "component", "api", "profile_id", identity.ProfileID, "key", identity.Key, "error", err) writeError(w, http.StatusInternalServerError, "internal_error", "Failed to store the setting") return false } @@ -1067,6 +1120,13 @@ func (h *SettingValuesHandler) deleteValueAt( return } } + // Both rows are gone, so the profile now inherits — and the legacy column + // has to say so too, or GET /profiles would go on serving the value this + // request just cleared. + if !h.syncLegacyIntroSkipColumn(r, w, store, identity, + h.legacyIntroSkipColumnCleared(identity.Key)) { + return + } auditSettingsForOther(r.Context(), settingsAuditRecord{ Action: "clear", ActorProfileID: actingProfileID(r.Context()), diff --git a/internal/api/handlers/settings_values_mirror_test.go b/internal/api/handlers/settings_values_mirror_test.go index f4230937..2ee13991 100644 --- a/internal/api/handlers/settings_values_mirror_test.go +++ b/internal/api/handlers/settings_values_mirror_test.go @@ -277,6 +277,97 @@ func TestProfileScopeEnumWriteUpdatesTheLegacyColumn(t *testing.T) { } } +// requireColumn asserts what user_profiles.auto_skip_intro holds, which is +// still what GET /profiles serves and what the web player reads. +func requireColumn(t *testing.T, store userstore.UserStore, want bool, when string) { + t.Helper() + profile, err := store.GetProfile(context.Background(), "profile-1") + if err != nil || profile == nil { + t.Fatalf("reading profile: %v", err) + } + if profile.AutoSkipIntro != want { + t.Errorf("user_profiles.auto_skip_intro = %v %s, want %v", + profile.AutoSkipIntro, when, want) + } +} + +// TestABooleanWriteCorrectsAColumnTheEnumMoved. The column has to be movable +// from both halves of the pair, or the first enum write pins it: an old client +// that then saves the switch stores its value in the canonical rows and reads +// the opposite back out of the profile DTO. +func TestABooleanWriteCorrectsAColumnTheEnumMoved(t *testing.T) { + handler, store := newValuesTestHandler(t) + + if rec := routeValues(t, handler, http.MethodPut, + settingskeys.PlaybackIntroSkipMode, "scope=profile", + []byte(`{"value":"always"}`)); rec.Code != http.StatusOK { + t.Fatalf("enum PUT = %d: %s", rec.Code, rec.Body.String()) + } + requireColumn(t, store, true, "after intro_skip_mode was set to always") + + if rec := routeValues(t, handler, http.MethodPut, + settingskeys.PlaybackAutoSkipIntro, "scope=profile", + []byte(`{"value":false}`)); rec.Code != http.StatusOK { + t.Fatalf("boolean PUT = %d: %s", rec.Code, rec.Body.String()) + } + requireStored(t, store, profileIdentity(settingskeys.PlaybackAutoSkipIntro), `false`) + requireStored(t, store, profileIdentity(settingskeys.PlaybackIntroSkipMode), `"ask"`) + requireColumn(t, store, false, "after the switch was turned off") +} + +// TestClearingTheProfileScopePairResetsTheLegacyColumn. A cleared row means +// "inherit again", so the column has to fall back with it — otherwise the +// profile DTO, and the web player that reads it, go on acting on the choice +// this request removed. +func TestClearingTheProfileScopePairResetsTheLegacyColumn(t *testing.T) { + for _, cleared := range []string{ + settingskeys.PlaybackIntroSkipMode, + settingskeys.PlaybackAutoSkipIntro, + } { + t.Run(cleared, func(t *testing.T) { + handler, store := newValuesTestHandler(t) + + if rec := routeValues(t, handler, http.MethodPut, + settingskeys.PlaybackIntroSkipMode, "scope=profile", + []byte(`{"value":"always"}`)); rec.Code != http.StatusOK { + t.Fatalf("seed PUT = %d: %s", rec.Code, rec.Body.String()) + } + requireColumn(t, store, true, "after the seeding write") + + if rec := routeValues(t, handler, http.MethodDelete, + cleared, "scope=profile", nil); rec.Code != http.StatusNoContent { + t.Fatalf("DELETE %s = %d: %s", cleared, rec.Code, rec.Body.String()) + } + // Nothing is stored at any scope now, so the pair resolves to the + // contract default — "ask", which the boolean spells false. + requireColumn(t, store, false, "after the profile-scope value was cleared") + }) + } +} + +// TestClearingADeviceOverrideLeavesTheProfileColumnAlone: the column follows the +// profile-wide choice, and giving up one television's override does not revoke +// it. +func TestClearingADeviceOverrideLeavesTheProfileColumnAlone(t *testing.T) { + handler, store := newValuesTestHandler(t) + + for _, scope := range []string{"scope=profile", "scope=profile_device"} { + if rec := routeValues(t, handler, http.MethodPut, + settingskeys.PlaybackIntroSkipMode, scope, + []byte(`{"value":"always"}`)); rec.Code != http.StatusOK { + t.Fatalf("seed PUT at %s = %d: %s", scope, rec.Code, rec.Body.String()) + } + } + requireColumn(t, store, true, "after the profile-scope write") + + if rec := routeValues(t, handler, http.MethodDelete, + settingskeys.PlaybackIntroSkipMode, "scope=profile_device", nil); rec.Code != http.StatusNoContent { + t.Fatalf("device DELETE = %d: %s", rec.Code, rec.Body.String()) + } + requireColumn(t, store, true, "after a device override was cleared") + requireStored(t, store, profileIdentity(settingskeys.PlaybackIntroSkipMode), `"always"`) +} + // TestDeviceScopeEnumWriteLeavesTheProfileColumnAlone: the column is // profile-wide, and one television's override is not the household's choice. func TestDeviceScopeEnumWriteLeavesTheProfileColumnAlone(t *testing.T) { diff --git a/web/src/lib/deviceSettingGroups.test.ts b/web/src/lib/deviceSettingGroups.test.ts index e917e8e8..e3e757c4 100644 --- a/web/src/lib/deviceSettingGroups.test.ts +++ b/web/src/lib/deviceSettingGroups.test.ts @@ -43,6 +43,16 @@ describe("deviceSettingGroups", () => { expect(groupForDeviceSetting("ui.library_page_state")).toBeNull(); }); + // playback.intro_skip_mode replaces playback.auto_skip_intro, which this + // player still reads, and the server keeps the pair in step at write time. + // Two controls for one preference on one screen would disagree with each + // other the moment either was touched, so only the switch shows until the + // player learns the mode. + it("shows one intro-skip control while both keys are live", () => { + expect(groupForDeviceSetting("playback.intro_skip_mode")).toBeNull(); + expect(groupForDeviceSetting("playback.auto_skip_intro")).toBe("episodes"); + }); + it("returns groups in reading order and omits empty ones", () => { const ids = groupDeviceSettings().map((group) => group.id); expect(ids).toEqual(["picture", "sound", "subtitles", "episodes"]); diff --git a/web/src/lib/deviceSettingGroups.ts b/web/src/lib/deviceSettingGroups.ts index 343c0a2e..eeb82d8d 100644 --- a/web/src/lib/deviceSettingGroups.ts +++ b/web/src/lib/deviceSettingGroups.ts @@ -43,6 +43,10 @@ const EXPLICIT_GROUPS: Partial> = { "playback.preferred_quality": "picture", "playback.max_bitrate_kbps": "picture", "playback.auto_skip_intro": "episodes", + // Hidden for now (see HIDDEN_KEYS), but its home is recorded here so that + // unhiding it is one deletion rather than a key that quietly falls through to + // the category fallback and lands under Picture. + "playback.intro_skip_mode": "episodes", "playback.auto_skip_credits": "episodes", "playback.auto_skip_recap": "episodes", "playback.auto_play_next": "episodes", @@ -68,6 +72,15 @@ const EXPLICIT_GROUPS: Partial> = { * screen, which already edits them at profile scope; showing them here would * give one setting two homes. `ui.library_page_state` is remembered browse * state rather than a preference — it has no control in the manifest at all. + * + * `playback.intro_skip_mode` is hidden for the length of one release rather + * than forever. It replaces `playback.auto_skip_intro`, which is still the key + * this player reads, so showing both would put two controls for one preference + * on one screen — and the server keeps the pair in step at write time, so + * editing either would silently move the other behind the UI. The web player + * cannot honour "never" yet either, so the three-way control would offer a mode + * that does nothing here. It appears, and the switch goes away, in the + * follow-up that teaches the player the mode. */ const HIDDEN_KEYS = new Set([ "ui.theme", @@ -78,6 +91,7 @@ const HIDDEN_KEYS = new Set([ "ui.remember_library_page_state", "nav.primary_menu", "ui.card_presentation", + "playback.intro_skip_mode", ]); /**