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", ]); /**