From f44240e5e5feb2685b20f0d5351a2df5763dc4f2 Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Mon, 27 Jul 2026 22:57:34 +0000 Subject: [PATCH] feat(settings): split quality into two axes and register the orphan keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two manifest changes the cutover needs. **Quality becomes resolution + bitrate.** The legacy ladder values (1080p-high, 720p-medium, 1080p-8, 420p, 328p) were never a third dimension — they are a bitrate spelled into the resolution string. The web player already decomposes them: useTranscodeQuality.ts defines 1080p-high as {resolution: 1080p, bitrate: 10000} and sends the two separately, so the compound form never reached the wire. Downloads went further and kept only a bitrate ladder. So playback.preferred_quality keeps the six clean resolutions and playback.max_bitrate_kbps becomes the second axis, nullable because "uncapped" is a real answer and a numeric sentinel would need widening every time hardware improves. Clients compose their own presets from the pair, which means retuning what "High" means is a client release rather than a contract break. Migration decomposes each legacy value losslessly, so none of them lands in the rejects table. **The five extension-bag keys are now definitions.** card_overlays, next_up_mode, sidebar_pins, disabled_library_ids and library_order reached the server only through the unknown-key path, stored as unvalidated strings. Two of them the server reads back — next_up_mode decides home section assembly and card_overlays falls back to an admin default — so they cannot be demoted to client-local. Registering them is what lets the extension bag close. Adds three schemas for their shapes and a test that exercises every schema_ref against a real value: each of these is nullable with a null default, so the existing default-validation test returns at the null branch without ever compiling the reference. Co-Authored-By: Claude Opus 5 (1M context) --- contracts/settings/v1/manifest.json | 112 +++++++++++++- .../settings/v1/schemas/card-overlays.json | 79 ++++++++++ .../settings/v1/schemas/library-id-list.json | 13 ++ .../settings/v1/schemas/sidebar-pins.json | 27 ++++ internal/settingscontract/contract_test.go | 143 ++++++++++++++++++ 5 files changed, 373 insertions(+), 1 deletion(-) create mode 100644 contracts/settings/v1/schemas/card-overlays.json create mode 100644 contracts/settings/v1/schemas/library-id-list.json create mode 100644 contracts/settings/v1/schemas/sidebar-pins.json diff --git a/contracts/settings/v1/manifest.json b/contracts/settings/v1/manifest.json index 4eb26d56..23040f50 100644 --- a/contracts/settings/v1/manifest.json +++ b/contracts/settings/v1/manifest.json @@ -140,7 +140,27 @@ "label": "Preferred quality", "description": "Pick the quality Silo should prefer.", "recommended_control": "select", - "notes": "Members are exactly the vocabulary the server already speaks: NormalizeQualityV3 in internal/playback/protocol_v3.go accepts auto, 480p, 720p, 1080p, 2160p and original and normalizes anything else to auto with a degradation warning. Transcode ladder rungs (328p, 720p-high, 1080p-8 and friends) are deliberately absent — they are encoder outputs, not user-facing choices, and registering them would have promised a preference the planner throws away. Members are listed ascending so the ceiling constraint has a defined direction; \"auto\" sorts lowest because it never exceeds a cap, and \"original\" highest because it is the uncapped source. Enforcing the ceiling needs internal/access/quality.go to learn both sentinels: qualityRank ranks neither today, so auto and original both tie at 0 with unset and a cap would let original through. That is Phase 2 work and the ordering it must implement is the one declared here. Migrates user_profiles.quality_preference as the profile fallback; because that column is NOT NULL DEFAULT '1080p', migration writes a profile row only where the value is not the column default, or every profile would be pinned to 1080p having never chosen it. The account/profile max_playback_quality columns stay in internal/policy and are NOT settings." + "notes": "The resolution axis. Members are exactly the vocabulary the server already speaks: NormalizeQualityV3 in internal/playback/protocol_v3.go accepts auto, 480p, 720p, 1080p, 2160p and original and normalizes anything else to auto with a degradation warning. Transcode ladder rungs (328p, 720p-high, 1080p-8 and friends) are deliberately absent — they were never a third dimension, only a bitrate spelled into the resolution string. web/src/player/hooks/useTranscodeQuality.ts already decomposes them, defining 1080p-high as {resolution: 1080p, bitrate: 10000} and sending the two to the server separately, so the compound form never reached the wire. playback.max_bitrate_kbps is now that second axis, and clients compose the two into whatever presets they want to show. Members are listed ascending so the ceiling constraint has a defined direction; \"auto\" sorts lowest because it never exceeds a cap, and \"original\" highest because it is the uncapped source. Enforcing the ceiling needs internal/access/quality.go to learn both sentinels: qualityRank ranks neither today, so auto and original both tie at 0 with unset and a cap would let original through. Migrates user_profiles.quality_preference as the profile fallback; because that column is NOT NULL DEFAULT '1080p', migration writes a profile row only where the value is not the column default, or every profile would be pinned to 1080p having never chosen it. The account/profile max_playback_quality columns stay in internal/policy and are NOT settings." + }, + { + "key": "playback.max_bitrate_kbps", + "introduced_in": 1, + "persistence": "remote", + "allowed_scopes": ["profile", "profile_device"], + "resolution_order": ["profile_device", "profile", "default"], + "value_schema": { + "type": "integer", + "nullable": true, + "minimum": 100, + "maximum": 200000 + }, + "default_value": null, + "unit": "kbps", + "category": "playback", + "label": "Maximum bitrate", + "description": "Cap how much bandwidth playback may use. No cap means Silo picks for the chosen resolution.", + "recommended_control": "select", + "notes": "The bitrate axis, orthogonal to playback.preferred_quality. Splitting them is what the clients were already doing: the in-player switcher sends resolution and bitrate as separate fields, and downloads (DownloadQuality in silo-android) dropped resolution entirely and kept only a bitrate ladder. Two values rather than one compound enum means a client can offer \"1080p High\" without the server having to agree on what \"High\" means — retuning a preset is a client release, not a contract break, and it stays additive under the widening rule. null is uncapped, which is why this is nullable rather than defaulting to a large number: absent and \"as much as you like\" are the same statement, and a numeric sentinel would have to be widened every time hardware improves. The bounds are deliberately loose — 100 kbps is below any watchable stream and 200 Mbps is above any remux — because this caps a preference, not a policy; entitlement limits live in internal/policy. Migration decomposes the legacy compound values: 1080p-high becomes (1080p, 10000), 720p-medium becomes (720p, 3000), 420p becomes (480p, 720), following the bitrates in web/src/player/hooks/useTranscodeQuality.ts, so no stored preference is lost to the rejects table." }, { "key": "playback.auto_skip_intro", @@ -615,6 +635,96 @@ "recommended_control": "select", "notes": "Moved from account to profile scope; the account row is copied to every profile during migration." }, + { + "key": "ui.card_overlays", + "introduced_in": 1, + "persistence": "remote", + "allowed_scopes": ["profile"], + "resolution_order": ["profile", "default"], + "value_schema": { + "type": "object", + "schema_ref": "card-overlays.json", + "nullable": true + }, + "default_value": null, + "platforms": ["web"], + "category": "appearance", + "label": "Poster badges", + "description": "Which badges appear on poster cards, and where.", + "notes": "Registered from the legacy unprefixed key card_overlays, which reached the server only through the unknown-key extension bag — stored as an arbitrary string with no validation. null means the user has expressed no preference, which is what lets the server-wide admin default in the overlay-config endpoint apply; writing a resolved-but-unchosen value would silently pin them. The admin default and the enabled kill switch stay in server_settings and are not user settings." + }, + { + "key": "ui.next_up_mode", + "introduced_in": 1, + "persistence": "remote", + "allowed_scopes": ["profile"], + "resolution_order": ["profile", "default"], + "value_schema": { + "type": "enum", + "values": [ + { "value": "combined", "label": "With Continue Watching" }, + { "value": "separate", "label": "Separate row" } + ] + }, + "default_value": "combined", + "category": "navigation", + "label": "Next up episodes", + "description": "Whether upcoming episodes stay with Continue Watching or get their own row.", + "recommended_control": "select", + "notes": "Registered from the legacy unprefixed key next_up_mode. The server reads it directly when assembling home sections, so it cannot be client-local; that read moves to the canonical resolver at cutover. The legacy value was untyped and absent meant combined, which is why combined is the default rather than a third \"unset\" member." + }, + { + "key": "ui.sidebar_pins", + "introduced_in": 1, + "persistence": "remote", + "allowed_scopes": ["profile"], + "resolution_order": ["profile", "default"], + "value_schema": { + "type": "object", + "schema_ref": "sidebar-pins.json", + "nullable": true + }, + "default_value": null, + "platforms": ["web"], + "category": "navigation", + "label": "Pinned sidebar items", + "description": "Sections and collections pinned into the sidebar.", + "notes": "Registered from the legacy unprefixed key sidebar_pins. Navigation state rather than an authored preference, so it has no control; it is written by the pin affordances themselves." + }, + { + "key": "ui.disabled_library_ids", + "introduced_in": 1, + "persistence": "remote", + "allowed_scopes": ["profile"], + "resolution_order": ["profile", "default"], + "value_schema": { + "type": "object", + "schema_ref": "library-id-list.json", + "nullable": true + }, + "default_value": null, + "category": "navigation", + "label": "Hidden libraries", + "description": "Libraries you have hidden from your own browsing.", + "notes": "Registered from the legacy unprefixed key disabled_library_ids. This is the user hiding a library from themselves — it is not an access control. Library visibility enforcement lives in internal/access and internal/policy, and nothing here may be read as a permission. Profile scope rather than profile_device because hiding a library is a statement about what you want to see, not about one screen." + }, + { + "key": "ui.library_order", + "introduced_in": 1, + "persistence": "remote", + "allowed_scopes": ["profile"], + "resolution_order": ["profile", "default"], + "value_schema": { + "type": "object", + "schema_ref": "library-id-list.json", + "nullable": true + }, + "default_value": null, + "category": "navigation", + "label": "Library order", + "description": "The order your libraries appear in.", + "notes": "Registered from the legacy unprefixed key library_order. Shares library-id-list.json with ui.disabled_library_ids: both are normalized by the same normalizeLibraryIDs in web/src/hooks/queries/libraries.ts, which drops non-integers and duplicates. A library id absent from the list sorts after the ones present, so a stale id for a deleted library is inert and needs no cleanup hook." + }, { "key": "downloads.wifi_only", "introduced_in": 1, diff --git a/contracts/settings/v1/schemas/card-overlays.json b/contracts/settings/v1/schemas/card-overlays.json new file mode 100644 index 00000000..120c4f0d --- /dev/null +++ b/contracts/settings/v1/schemas/card-overlays.json @@ -0,0 +1,79 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://silo-server.dev/contracts/settings/v1/schemas/card-overlays.json", + "title": "Card overlay preferences", + "description": "Badges painted on poster cards. Mirrors CardOverlayPrefs in web/src/lib/overlays/types.ts.", + "type": "object", + "additionalProperties": false, + "required": ["version", "preset", "order", "items"], + "properties": { + "version": { "const": 2 }, + "preset": { + "type": "string", + "enum": ["minimal", "classic", "vibrant", "pill", "square"] + }, + "order": { + "description": "Explicit render order. Empty means use the registry's own order.", + "type": "array", + "maxItems": 64, + "items": { "$ref": "#/$defs/overlayId" } + }, + "items": { + "type": "object", + "propertyNames": { "$ref": "#/$defs/overlayId" }, + "additionalProperties": { + "type": "object", + "additionalProperties": false, + "required": ["enabled", "position"], + "properties": { + "enabled": { "type": "boolean" }, + "position": { + "type": "string", + "enum": ["top-left", "top-right", "bottom-left", "bottom-right"] + }, + "accentColor": { + "description": "Hex colour. Absent means the overlay's own default accent.", + "type": "string", + "pattern": "^#[0-9a-fA-F]{6}$" + }, + "showIcon": { + "description": "Absent means inherit from the preset.", + "type": "boolean" + } + } + } + } + }, + "$defs": { + "overlayId": { + "type": "string", + "enum": [ + "resolution", + "hdr", + "resolution_hdr", + "audio", + "audio_channels", + "video_codec", + "container", + "aspect_ratio", + "release_type", + "edition", + "multi_audio", + "multi_sub", + "rating_imdb", + "rating_tmdb", + "rating_rt", + "rating_rt_audience", + "content_rating", + "year", + "runtime", + "original_language", + "studio", + "network", + "show_status", + "imdb_top_250", + "rt_certified_fresh" + ] + } + } +} diff --git a/contracts/settings/v1/schemas/library-id-list.json b/contracts/settings/v1/schemas/library-id-list.json new file mode 100644 index 00000000..3878cc7f --- /dev/null +++ b/contracts/settings/v1/schemas/library-id-list.json @@ -0,0 +1,13 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://silo-server.dev/contracts/settings/v1/schemas/library-id-list.json", + "title": "Library id list", + "description": "An ordered, duplicate-free list of library ids. Backs both ui.disabled_library_ids and ui.library_order; the web client normalizes with normalizeLibraryIDs in web/src/hooks/queries/libraries.ts, which drops non-integers and anything below 1.", + "type": "array", + "maxItems": 512, + "uniqueItems": true, + "items": { + "type": "integer", + "minimum": 1 + } +} diff --git a/contracts/settings/v1/schemas/sidebar-pins.json b/contracts/settings/v1/schemas/sidebar-pins.json new file mode 100644 index 00000000..375f172d --- /dev/null +++ b/contracts/settings/v1/schemas/sidebar-pins.json @@ -0,0 +1,27 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://silo-server.dev/contracts/settings/v1/schemas/sidebar-pins.json", + "title": "Sidebar pins", + "description": "Sections and collections pinned into the sidebar, grouped by the library they belong to. Mirrors SidebarPins in web/src/api/types.ts.", + "type": "object", + "propertyNames": { + "description": "The group the pins sit under — a library id, or a well-known group name.", + "type": "string", + "maxLength": 64 + }, + "maxProperties": 512, + "additionalProperties": { + "type": "array", + "maxItems": 128, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["type", "id", "label"], + "properties": { + "type": { "type": "string", "enum": ["section", "collection"] }, + "id": { "type": "string", "minLength": 1, "maxLength": 128 }, + "label": { "type": "string", "maxLength": 256 } + } + } + } +} diff --git a/internal/settingscontract/contract_test.go b/internal/settingscontract/contract_test.go index 8f649193..43e49eb0 100644 --- a/internal/settingscontract/contract_test.go +++ b/internal/settingscontract/contract_test.go @@ -95,6 +95,16 @@ func TestEveryCurrentServerKeyIsRegistered(t *testing.T) { "player.sleep_timer_default_minutes": "player.sleep_timer_default_minutes", "player.next_up_prompt_seconds": keyNextUpPrompt, + // Unregistered keys the web client writes through the extension bag. + // Two of these the server also reads back, so they cannot be demoted to + // client-local state: next_up_mode decides home section assembly, and + // card_overlays falls back to a server-wide admin default. + "card_overlays": "ui.card_overlays", + "next_up_mode": "ui.next_up_mode", + "sidebar_pins": "ui.sidebar_pins", + "disabled_library_ids": "ui.disabled_library_ids", + "library_order": "ui.library_order", + // Profile columns that become settings. "user_profiles.language": keyAudioLanguage, "user_profiles.subtitle_language": "playback.subtitle_language", @@ -1345,3 +1355,136 @@ func TestLibraryPageStateAcceptsTheWebClientsRealSearchStrings(t *testing.T) { t.Error("an 8KiB search string was accepted; the bound is not enforced") } } + +// TestObjectSchemasAcceptTheShapesClientsStore exercises every schema_ref +// against a real value. +// +// Nothing else does. Each of these definitions is nullable with a null default, +// so TestDefaultsValidateAgainstTheirOwnSchema returns at the null branch +// without ever compiling the reference — a schema_ref naming a file that does +// not exist, or a schema that rejects the shape its client actually stores, +// would pass every other test in this file. +func TestObjectSchemasAcceptTheShapesClientsStore(t *testing.T) { + manifest, err := Load() + if err != nil { + t.Fatalf("loading manifest: %v", err) + } + + valid := map[string]string{ + "ui.card_overlays": `{"version":2,"preset":"minimal","order":["hdr","year"],` + + `"items":{"hdr":{"enabled":true,"position":"top-left","accentColor":"#f5c518"},` + + `"year":{"enabled":false,"position":"bottom-right","showIcon":true}}}`, + "ui.sidebar_pins": `{"7":[{"type":"section","id":"recently-added","label":"Recently Added"}],` + + `"12":[{"type":"collection","id":"c-9","label":"Marvel"}]}`, + "ui.disabled_library_ids": `[3,9]`, + "ui.library_order": `[9,3,1]`, + "playback.subtitle_appearance": `{"fontSize":"large","position":"top"}`, + "ui.custom_theme_vars": `{"color-bg":"#101014"}`, + } + for key, value := range valid { + t.Run(key, func(t *testing.T) { + def, ok := manifest.Lookup(key) + if !ok { + t.Fatalf("%s is not registered", key) + } + if err := def.ValueSchema.ValidateValue(json.RawMessage(value), objSchemas); err != nil { + t.Fatalf("a value the client stores was rejected: %v", err) + } + }) + } + + // Each schema must actually constrain something, or it is decoration. + invalid := map[string]string{ + "ui.card_overlays": `{"version":2,"preset":"nonesuch","order":[],"items":{}}`, + "ui.sidebar_pins": `{"7":[{"type":"bogus","id":"x","label":"L"}]}`, + "ui.disabled_library_ids": `[0,-1]`, + "ui.library_order": `[1,1]`, + } + for key, value := range invalid { + t.Run(key+" rejects", func(t *testing.T) { + def, ok := manifest.Lookup(key) + if !ok { + t.Fatalf("%s is not registered", key) + } + if err := def.ValueSchema.ValidateValue(json.RawMessage(value), objSchemas); err == nil { + t.Fatalf("%s was accepted", value) + } + }) + } +} + +// TestQualityIsTwoIndependentAxes pins the split that replaced the compound +// ladder values. A client composes its own presets from these two, so the +// contract must not constrain them jointly. +func TestQualityIsTwoIndependentAxes(t *testing.T) { + manifest, err := Load() + if err != nil { + t.Fatalf("loading manifest: %v", err) + } + quality, ok := manifest.Lookup(keyPreferredQuality) + if !ok { + t.Fatal("playback.preferred_quality is not registered") + } + bitrate, ok := manifest.Lookup("playback.max_bitrate_kbps") + if !ok { + t.Fatal("playback.max_bitrate_kbps is not registered") + } + + // The resolution axis carries no bitrate spellings. + for _, member := range quality.ValueSchema.Values { + if s, isString := member.Value.(string); isString && strings.Contains(s, "-") { + t.Errorf("quality member %q looks like a compound ladder rung; "+ + "bitrate belongs to playback.max_bitrate_kbps", s) + } + } + + // Uncapped has to be expressible, or every client invents a sentinel. + if !bitrate.ValueSchema.Nullable { + t.Error("max_bitrate_kbps must be nullable so uncapped is a real value") + } + if string(bitrate.DefaultValue) != jsonNull { + t.Errorf("max_bitrate_kbps defaults to %s, want null (uncapped)", bitrate.DefaultValue) + } + + // Both axes must resolve identically, or a device override of one and a + // profile value of the other would compose into a pair the user never chose. + if len(quality.ResolutionOrder) != len(bitrate.ResolutionOrder) { + t.Fatalf("resolution orders differ: %v vs %v", + quality.ResolutionOrder, bitrate.ResolutionOrder) + } + for i := range quality.ResolutionOrder { + if quality.ResolutionOrder[i] != bitrate.ResolutionOrder[i] { + t.Errorf("resolution orders differ at %d: %q vs %q", + i, quality.ResolutionOrder[i], bitrate.ResolutionOrder[i]) + } + } + + // The legacy compound values decompose losslessly; this is the table the + // migration implements. + for _, tc := range []struct { + legacy string + resolution string + kbps int + }{ + {"1080p-high", "1080p", 10000}, + {"1080p", "1080p", 6000}, + {"1080p-8", "1080p", 6000}, + {"720p-high", "720p", 4000}, + {"720p-medium", "720p", 3000}, + {"720p", "720p", 2000}, + {"480p", "480p", 1500}, + {"420p", "480p", 720}, + {"328p", "480p", 720}, + } { + t.Run("decompose "+tc.legacy, func(t *testing.T) { + res, _ := json.Marshal(tc.resolution) + if err := quality.ValueSchema.ValidateValue(res, objSchemas); err != nil { + t.Errorf("resolution %q is not a member: %v", tc.resolution, err) + } + kbps, _ := json.Marshal(tc.kbps) + if err := bitrate.ValueSchema.ValidateValue(kbps, objSchemas); err != nil { + t.Errorf("bitrate %d is out of range: %v", tc.kbps, err) + } + }) + } +}