diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 00000000..b331bc4b --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,107 @@ +name: CI + +on: + pull_request: + push: + branches: + - main + workflow_dispatch: + +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true + +env: + GOPROXY: https://proxy.golang.org,direct + GOPRIVATE: github.com/Silo-Server/* + GONOSUMDB: github.com/Silo-Server/* + +jobs: + go: + name: Go + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v5 + + - name: Set up Go + uses: actions/setup-go@v6 + with: + go-version-file: go.mod + cache: true + + # cmd/silo embeds the built frontend, so nothing under ./... compiles + # without web/dist. The Go jobs never serve it, so a placeholder is + # enough; the Docker workflow builds the real bundle. + - name: Stub the embedded frontend bundle + run: make embed-stub + + - name: Build + run: go build ./... + + - name: gofmt + run: | + unformatted="$(gofmt -l .)" + if [ -n "$unformatted" ]; then + echo "::error::gofmt is required on:" + echo "$unformatted" + exit 1 + fi + + - name: Vet + run: go vet ./... + + # Runs the settings-contract gate among everything else: the embedded + # manifest must parse, satisfy its own schema, hold every structural + # invariant, and agree with the live settings registry on keys and + # defaults. Without this job those tests exist but never run. + - name: Test + run: make test-go + + web: + name: Web + runs-on: ubuntu-latest + defaults: + run: + working-directory: web + steps: + - name: Checkout + uses: actions/checkout@v5 + + - name: Set up pnpm + uses: pnpm/action-setup@v4 + + - name: Set up Node + uses: actions/setup-node@v5 + with: + node-version: 22 + cache: pnpm + cache-dependency-path: web/pnpm-lock.yaml + + - name: Install + run: pnpm install --frozen-lockfile + + - name: Lint + run: pnpm run lint + + - name: Format check + run: pnpm run format:check + + - name: Typecheck and build + run: pnpm run build + + # Includes the appearance-cache ownership tests, which are the regression + # guard for cross-account leaks in the localStorage warm start. + - name: Test + working-directory: . + run: make test-web + + docs: + name: Docs hygiene + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v5 + + - name: Verify no local paths leaked into committed docs + run: make verify-local-paths diff --git a/AGENTS.md b/AGENTS.md index 5ee0e824..427b5496 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,18 +71,25 @@ SDK, in the catalog, or in a specific plugin repo. ## Building and verifying -`make build`, `make dev-backend`, `make dev-frontend`, `make lint`, `make migrate-status` / -`make migrate-up` — read the `Makefile` for the rest. Local services: +`make build`, `make dev-backend`, `make dev-frontend`, `make lint`, `make test`, `make migrate-status` +/ `make migrate-up` — read the `Makefile` for the rest. Local services: `docker compose up -d postgres redis`. +`make test-go` skips the tests named in `GOTEST_KNOWN_FAILURES`, which are failures that predate the +CI gate and are tracked separately. That list may only shrink: delete an entry together with its +fix, and never add to it to make a new change pass. + Before opening a merge request: ```bash make lint +make test cd web && pnpm run lint && pnpm run format:check make verify-local-paths ``` +`.github/workflows/ci.yml` runs the same checks on every pull request. + Go stays `gofmt`/`goimports` clean; the frontend follows `web/.prettierrc`. ## Skills diff --git a/Makefile b/Makefile index de6ef35e..24b90b59 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: frontend build dev-frontend dev-backend dev-proxy dev-transcode lint clean jellyfin-web migrate-continuum-check verify-local-paths install-hooks migrate-create migrate-validate migrate-status migrate-up +.PHONY: frontend build dev-frontend dev-backend dev-proxy dev-transcode lint test test-go test-web embed-stub clean jellyfin-web migrate-continuum-check verify-local-paths install-hooks migrate-create migrate-validate migrate-status migrate-up GIT_COMMON_DIR := $(strip $(shell git rev-parse --git-common-dir 2>/dev/null)) MAIN_CHECKOUT_ROOT := $(if $(GIT_COMMON_DIR),$(abspath $(GIT_COMMON_DIR)/..)) @@ -54,6 +54,38 @@ lint: golangci-lint run cd web && pnpm run lint +# Tests that fail on main today and are tracked separately. Nothing in this list +# is owned by the change that added it — the point of naming them individually, +# rather than skipping whole packages, is that everything else stays gated and +# the list can only shrink. Delete an entry along with its fix; do not add. +GOTEST_KNOWN_FAILURES := TestHandleReplanPlaybackV3SeekFailureRecoveryNeverChangesMediaVersion|TestAutoscanMediaUpdatedIgnoresUnsupportedSidecars|TestAutoscanMediaUpdatedSidecarsScanParent|TestAutoscanMediaUpdatedRootSidecarDoesNotScanLibrary + +# Frontend test files that fail on main today. Same rules as +# GOTEST_KNOWN_FAILURES: shrink-only, and never extend it to land a change. +WEBTEST_KNOWN_FAILURES := \ + --exclude src/pages/Catalog.test.tsx \ + --exclude src/pages/ItemDetail/SeasonContent.test.tsx \ + --exclude src/pages/LibraryRecommended.test.tsx \ + --exclude src/pages/audiobooks/player/useAudiobookPlayback.test.ts \ + --exclude src/pages/setup-wizard/steps/ServerStorageStep.test.tsx \ + --exclude src/player/hooks/useASSSubtitles.test.tsx + +# The Go binary embeds the built frontend, so every Go build and test needs +# web/dist to exist. Tests never serve it, so a placeholder is enough; `make +# build` still builds the real bundle. +embed-stub: + @mkdir -p web/dist + @[ -e web/dist/index.html ] || printf '\n' > web/dist/index.html + +# Run the Go and frontend test suites. +test: test-go test-web + +test-go: embed-stub + go test -skip '$(GOTEST_KNOWN_FAILURES)' ./... + +test-web: + cd web && pnpm exec vitest run $(WEBTEST_KNOWN_FAILURES) + # Check committed content for local machine path leaks. verify-local-paths: scripts/check-local-path-leaks.sh diff --git a/cmd/silo/main.go b/cmd/silo/main.go index 8c4da0a6..bb5f4a7a 100644 --- a/cmd/silo/main.go +++ b/cmd/silo/main.go @@ -95,6 +95,7 @@ import ( "github.com/Silo-Server/silo-server/internal/secret" "github.com/Silo-Server/silo-server/internal/sections" "github.com/Silo-Server/silo-server/internal/server" + "github.com/Silo-Server/silo-server/internal/settingscontract" "github.com/Silo-Server/silo-server/internal/subtitles" "github.com/Silo-Server/silo-server/internal/taskmanager" taskrepository "github.com/Silo-Server/silo-server/internal/taskmanager/repository" @@ -395,6 +396,21 @@ func main() { ctx := context.Background() + // Step 0: Validate the embedded settings contract before anything can + // depend on it. A malformed or self-inconsistent manifest is a build defect, + // not a runtime condition, so failing here — loudly, before the first + // request — is the whole point: the alternative is shipping an image whose + // contract disagrees with the clients that vendored it. + contract := settingscontract.MustLoad() + contractETag, err := settingscontract.ETag() + if err != nil { + log.Fatalf("settings contract: %v", err) + } + slog.Info("settings contract loaded", + "revision", contract.Revision, + "definitions", len(contract.Definitions), + "etag", contractETag) + // Step 1: Bootstrap from .env bc, err := config.LoadBootstrap(*envFile) if err != nil { diff --git a/contracts/settings/v1/manifest.json b/contracts/settings/v1/manifest.json index 754690c8..e3d5ee33 100644 --- a/contracts/settings/v1/manifest.json +++ b/contracts/settings/v1/manifest.json @@ -82,12 +82,12 @@ "default" ], "value_schema": { "type": "boolean" }, - "default_value": false, + "default_value": true, "category": "playback", "label": "Show forced subtitles", "description": "Show subtitles for foreign-language dialogue even when subtitles are off.", "recommended_control": "switch", - "notes": "The existing Has* companion booleans on LibraryPlaybackPreference and SubtitlePreference encode set-vs-unset. Migration writes a row only where Has* is true, so explicit false stays distinct from unset." + "notes": "Default is true because that is what the server resolves today: user_profiles.show_forced_subtitles is NOT NULL DEFAULT true (migration 029) and profile creation sets it true. A false default here would silently turn forced subtitles off for every profile that never touched the toggle. The Has* companion booleans on LibraryPlaybackPreference and SubtitlePreference encode set-vs-unset at the library and series scopes, so migration writes rows there only where Has* is true. The profile column has no companion and cannot distinguish an explicit true from the column default, so migration writes a profile row only where the value is false — the value that differs from the default." }, { "key": "playback.subtitle_appearance", @@ -111,7 +111,7 @@ "label": "Subtitle appearance", "description": "How subtitles are drawn during playback.", "recommended_control": "panel", - "notes": "Renamed from the unprefixed legacy key \"subtitle_appearance\". Every other canonical key carries a domain prefix, and preserving accidental key names is an explicit non-goal of the design. Migration copies the account-level legacy fallback to every existing profile and leaves device overrides unchanged." + "notes": "Renamed from the unprefixed legacy key \"subtitle_appearance\". Every other canonical key carries a domain prefix, and preserving accidental key names is an explicit non-goal of the design. The rename touches three URL paths in internal/api/router.go, the admin device-settings routes, and the key constant in every client, so it cannot land without them. Migration copies the account-level legacy fallback to every existing profile and leaves device overrides unchanged, rewriting the key on each row. The default below is the web client's; Apple defaults to a box background and Android to no background with an outline, so migration must first write each platform's own default into a row for users who never opened the panel, or their subtitles silently change appearance at cutover." }, { "key": "playback.preferred_quality", @@ -124,17 +124,10 @@ "ordered": true, "values": [ { "value": "auto", "label": "Auto" }, - { "value": "328p" }, - { "value": "420p" }, - { "value": "480p" }, - { "value": "720p" }, - { "value": "720p-medium" }, - { "value": "720p-high" }, - { "value": "1080p-8" }, - { "value": "1080p" }, - { "value": "1080p-medium" }, - { "value": "1080p-high" }, - { "value": "2160p" }, + { "value": "480p", "label": "480p" }, + { "value": "720p", "label": "720p" }, + { "value": "1080p", "label": "1080p" }, + { "value": "2160p", "label": "2160p / 4K" }, { "value": "original", "label": "Original quality" } ] }, @@ -147,7 +140,7 @@ "label": "Preferred quality", "description": "Pick the quality Silo should prefer.", "recommended_control": "select", - "notes": "Members are listed ascending so the ceiling constraint has a defined direction; \"auto\" sorts lowest because it never exceeds a cap. Migrates user_profiles.quality_preference as the profile fallback. The account/profile max_playback_quality columns stay in internal/policy and are NOT settings." + "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." }, { "key": "playback.auto_skip_intro", @@ -237,15 +230,11 @@ "resolution_order": ["profile", "default"], "value_schema": { "type": "language_tag", "nullable": true }, "default_value": null, - "constrained_by": { - "policy_input": "profile_preferred_metadata_language", - "constraint": "allowlist" - }, "category": "catalog", "label": "Metadata language", "description": "Language Silo prefers for titles, descriptions, and artwork.", "recommended_control": "select", - "notes": "Migrates user_profiles.preferred_metadata_language." + "notes": "Migrates user_profiles.preferred_metadata_language; that column is NOT NULL DEFAULT '', and the empty string means unset, so migration writes a row only where it is non-empty. Deliberately carries no constrained_by. An earlier draft declared an allowlist on policy input profile_preferred_metadata_language, which is circular: internal/policy/input.go populates that field from this very column and vendor/scope.rego relays it unchanged as a preference. Policy narrows nothing here, and an allowlist bound to a scalar equal to the current value would either be a no-op or reject every change the user makes." }, { "key": "player.hdr_enabled", @@ -313,7 +302,7 @@ "label": "Match content frame rate", "description": "Switch the display refresh rate to match what is playing.", "recommended_control": "switch", - "notes": "Previously an unregistered Android device-setting write, so every write and reset was rejected by the server. Registered here." + "notes": "Android keeps this device-local today: it is absent from PlaybackSettingsKeys.DeviceSettings and documented there as deliberately not synced, so it was never written to the server rather than written and rejected. Registered here because a display-matching preference belongs to the device and should follow a profile across reinstalls." }, { "key": "player.playback_speed", @@ -328,7 +317,7 @@ "label": "Playback speed", "description": "Default playback speed on this device.", "recommended_control": "slider", - "notes": "Android currently clamps to 4.0. The contract maximum is 3.0, matching the server; the Android clamp and its unit test are corrected to match." + "notes": "Range matches the server and the shipped clients: Android already clamps to 0.25..3.0 and no picker offers above 3.0. The 0.05 step is enforced by ValidateValue, not just advertised, so every client's stepper lands on values the server accepts." }, { "key": "player.audio_sync_ms", @@ -404,14 +393,14 @@ "persistence": "remote", "allowed_scopes": ["profile_device"], "resolution_order": ["profile_device", "default"], - "value_schema": { "type": "integer", "minimum": 0, "maximum": 480 }, + "value_schema": { "type": "integer", "minimum": 0, "maximum": 240 }, "default_value": 0, "unit": "minutes", "category": "player", "label": "Default sleep timer", "description": "Preset duration for the sleep timer. 0 leaves it off.", "recommended_control": "stepper", - "notes": "Previously an unregistered Android device-setting write. The design classes transient sleep timers as private local; the persisted default duration is a real preference, so it is registered while the running timer stays client-local." + "notes": "Android keeps this device-local today and clamps to 0..240; it was never written to the server rather than written and rejected. The maximum matches that clamp rather than exceeding it, because a manifest bound no shipped client can produce is the drift this contract exists to remove. Raising it later is additive under the widening rule: bump maximum and tag it with maximum_introduced_in. The design classes a running sleep timer as private local; only the persisted default duration is a preference, so only it is registered." }, { "key": "ui.theme", @@ -435,7 +424,7 @@ "label": "Theme", "description": "Colour theme for the Silo interface.", "recommended_control": "select", - "notes": "Renamed from the unregistered legacy key \"ui_theme\", which the extension bag accepted without validation. Moved from account to profile scope: appearance is per household member, and the account row is copied to every profile during migration. Carries a device override because the right theme is partly a function of the screen and the room — a light theme on a phone in daylight, a dark one on a TV at night — which is the same reasoning that gives ui.text_scale one. Note that ui.custom_theme_vars and ui.custom_css stay profile-wide, so a profile's custom styling still applies on top of a device's theme override. Adding a theme is an additive enum widening. The admin-set default theme stays in server_settings and is not a user setting." + "notes": "Renamed from the unregistered legacy key \"ui_theme\", which the extension bag accepted without validation. Moved from account to profile scope: appearance is per household member, and the account row is copied to every profile during migration. Carries a device override because the right theme is partly a function of the screen and the room — a light theme on a phone in daylight, a dark one on a TV at night — which is the same reasoning that gives ui.text_scale one. Note that ui.custom_theme_vars and ui.custom_css stay profile-wide, so a profile's custom styling still applies on top of a device's theme override. Adding a theme is an additive enum widening. The admin-set default theme stays in server_settings and is not a user setting. Migration must also update internal/plugins/user_theme_lookup.go, which reads this value with raw SQL bound to both the old name and the account scope (SELECT value FROM user_settings WHERE user_id = $1 AND key = 'ui_theme') and feeds the X-Silo-Theme header on every plugin request. Left alone, that query matches nothing after the rename and every plugin UI silently falls back to its own theme, with no error to notice." }, { "key": "ui.text_scale", @@ -656,6 +645,32 @@ "recommended_control": "switch", "notes": "Contract-known local. Governs on-device storage cleanup prompts." }, + { + "key": "downloads.default_quality", + "introduced_in": 1, + "persistence": "client_local", + "allowed_scopes": ["client_local"], + "resolution_order": ["client_local", "default"], + "value_schema": { + "type": "enum", + "ordered": true, + "values": [ + { "value": "1mbps", "label": "1 Mbps" }, + { "value": "2mbps", "label": "2 Mbps" }, + { "value": "5mbps", "label": "5 Mbps" }, + { "value": "10mbps", "label": "10 Mbps" }, + { "value": "20mbps", "label": "20 Mbps" }, + { "value": "original", "label": "Original" } + ] + }, + "default_value": "original", + "platforms": ["ios", "android"], + "category": "downloads", + "label": "Download quality", + "description": "Quality preset used for new downloads.", + "recommended_control": "select", + "notes": "Contract-known local: the value is chosen on the device holding the files and is sent on each POST /downloads rather than stored server-side. Members are the DownloadQuality wire presets, ascending. Registered as client_local rather than left unregistered because it is a user-facing preference with shared semantics, and the manifest's invariant is that no production setting exists without an entry." + }, { "key": "subtitle.matches_device", "introduced_in": 1, @@ -669,7 +684,69 @@ "label": "Match device caption settings", "description": "Use the operating system's caption style instead of Silo's.", "recommended_control": "switch", - "notes": "Contract-known local: reads OS accessibility settings that only exist on the device. When enabled, playback.subtitle_appearance is not applied. Apple's existing copy separating this from profile subtitle behavior is the UX baseline." + "notes": "Contract-known local: reads OS accessibility settings that only exist on the device. When enabled, playback.subtitle_appearance is not applied. Apple's existing copy separating this from profile subtitle behavior is the UX baseline. A contract key names a setting; it is not a storage key. Clients keep whatever local key they already use — Android stores this at subtitle.matches_device.local, Apple at player.subtitleMatchesSystemAppearance — so adopting the contract does not reset anyone's local preferences. The same applies to downloads.wifi_only and downloads.keep_watched, which Apple stores as downloads.wifiOnly and downloads.keepWatchedDownloads." + }, + { + "key": "player.resume_rewind_seconds", + "introduced_in": 1, + "persistence": "client_local", + "allowed_scopes": ["client_local"], + "resolution_order": ["client_local", "default"], + "value_schema": { "type": "integer", "minimum": 0, "maximum": 30 }, + "default_value": 7, + "unit": "seconds", + "platforms": ["ios", "tvos", "macos", "android", "android_tv", "web"], + "category": "player", + "label": "Rewind on resume", + "description": "Skip back this far when resuming a partly watched item, to re-establish context. 0 turns it off.", + "recommended_control": "stepper", + "notes": "Contract-known local: it tunes playback feel on the device doing the playing. Registered so the name, range and default are shared rather than reinvented per platform." + }, + { + "key": "player.passout_threshold", + "introduced_in": 1, + "persistence": "client_local", + "allowed_scopes": ["client_local"], + "resolution_order": ["client_local", "default"], + "value_schema": { "type": "integer", "minimum": 0, "maximum": 20 }, + "default_value": 3, + "unit": "episodes", + "platforms": ["ios", "tvos", "macos", "android", "android_tv", "web"], + "category": "player", + "label": "Still watching prompt", + "description": "How many episodes auto-play before Silo asks whether you are still watching. 0 never asks.", + "recommended_control": "stepper", + "notes": "Contract-known local: pass-out protection counts consecutive auto-advances in one client session, which no other device can observe." + }, + { + "key": "player.picture_in_picture_enabled", + "introduced_in": 1, + "persistence": "client_local", + "allowed_scopes": ["client_local"], + "resolution_order": ["client_local", "default"], + "value_schema": { "type": "boolean" }, + "default_value": true, + "platforms": ["ios", "macos", "android"], + "category": "player", + "label": "Picture in picture", + "description": "Keep playing in a floating window when you leave the player.", + "recommended_control": "switch", + "notes": "Contract-known local: picture-in-picture is an OS capability of the device, not a playback preference the server resolves." + }, + { + "key": "nav.show_audiobooks", + "introduced_in": 1, + "persistence": "client_local", + "allowed_scopes": ["client_local"], + "resolution_order": ["client_local", "default"], + "value_schema": { "type": "boolean" }, + "default_value": false, + "platforms": ["ios", "tvos", "macos", "android", "android_tv"], + "category": "nav", + "label": "Show audiobooks", + "description": "Show the Audiobooks section in navigation.", + "recommended_control": "switch", + "notes": "Contract-known local: an opt-in navigation surface, hidden by default, with existing Apple (AppNavPreferences.showAudiobooks) and Android parity. Android stores it locally at nav.show_audiobooks.local." } ] } diff --git a/contracts/settings/v1/schemas/subtitle-appearance.json b/contracts/settings/v1/schemas/subtitle-appearance.json index 26f1f969..dd5eb040 100644 --- a/contracts/settings/v1/schemas/subtitle-appearance.json +++ b/contracts/settings/v1/schemas/subtitle-appearance.json @@ -2,7 +2,7 @@ "$schema": "https://json-schema.org/draft/2020-12/schema", "$id": "https://silo-server.dev/contracts/settings/v1/schemas/subtitle-appearance.json", "title": "Subtitle appearance", - "description": "Rendering appearance for subtitle tracks. Mirrors web/src/lib/subtitleAppearance.ts.", + "description": "Rendering appearance for subtitle tracks. Shared by the web, Apple and Android players; where they disagree the wider vocabulary wins, because a value a shipped client can already produce must stay storable.", "type": "object", "additionalProperties": false, "required": [ @@ -22,8 +22,11 @@ "enum": ["small", "medium", "large", "xlarge", "xxlarge"] }, "fontFamily": { + "description": "A font family name. Not an enum: the Apple clients offer every family CTFontManagerCopyAvailableFontFamilyNames reports and store the chosen name verbatim, so restricting this to the web's three generic families would invalidate the stored appearance of every user who picked a real font. The pattern bounds it to a plain family name — no separators, quotes, parentheses or braces — so a value is safe to interpolate into CSS or a platform font lookup.", "type": "string", - "enum": ["sans-serif", "serif", "monospace"] + "minLength": 1, + "maxLength": 64, + "pattern": "^[A-Za-z0-9][A-Za-z0-9 ._-]*$" }, "fontColor": { "$ref": "#/$defs/hexColor" }, "backgroundColor": { "$ref": "#/$defs/hexColor" }, diff --git a/internal/api/handlers/settings_contract_test.go b/internal/api/handlers/settings_contract_test.go new file mode 100644 index 00000000..cd9c566f --- /dev/null +++ b/internal/api/handlers/settings_contract_test.go @@ -0,0 +1,141 @@ +package handlers + +import ( + "encoding/json" + "strconv" + "strings" + "testing" + + "github.com/Silo-Server/silo-server/internal/settingscontract" +) + +// contractKeyRenames maps a legacy registry key to the canonical contract key +// where the two deliberately differ. +// +// This is the only handwritten part of the cross-check, and it encodes a +// decision rather than an inventory: every entry is a rename the manifest notes +// justify. The registry itself is iterated, never transcribed — a hand-copied +// key list cannot detect a key added to one side and not the other, which is +// the drift this whole contract exists to prevent. +var contractKeyRenames = map[string]string{ + "subtitle_appearance": "playback.subtitle_appearance", +} + +func canonicalContractKey(registryKey string) string { + if canonical, ok := contractKeyRenames[registryKey]; ok { + return canonical + } + return registryKey +} + +// TestEverySettingsRegistryKeyIsRegisteredInTheContract is the gate that makes +// the manifest authoritative rather than descriptive. Adding a key to +// settingsRegistry without a manifest definition fails here. +func TestEverySettingsRegistryKeyIsRegisteredInTheContract(t *testing.T) { + manifest, err := settingscontract.Load() + if err != nil { + t.Fatalf("loading settings contract: %v", err) + } + + for registryKey := range settingsRegistry { + canonical := canonicalContractKey(registryKey) + if _, ok := manifest.Lookup(canonical); !ok { + t.Errorf("settingsRegistry key %q has no definition in "+ + "contracts/settings/v1/manifest.json (looked up %q). Add one, or add a "+ + "rename to contractKeyRenames if the canonical name differs.", + registryKey, canonical) + } + } +} + +// TestContractRenamesStayLive keeps the rename table honest: an entry for a key +// the registry no longer has is dead weight that hides the next real rename. +func TestContractRenamesStayLive(t *testing.T) { + for registryKey := range contractKeyRenames { + if _, ok := settingsRegistry[registryKey]; !ok { + t.Errorf("contractKeyRenames maps %q, which settingsRegistry no longer defines", + registryKey) + } + } +} + +// TestRegistryDefaultsMatchTheContract catches the failure mode that is silent +// in production: the two sides agree a setting exists and disagree on what it +// resolves to when nobody has set it. A user who never touched the toggle gets +// one answer from the server today and a different one from a manifest-driven +// client tomorrow. +func TestRegistryDefaultsMatchTheContract(t *testing.T) { + manifest, err := settingscontract.Load() + if err != nil { + t.Fatalf("loading settings contract: %v", err) + } + + for registryKey, spec := range settingsRegistry { + canonical := canonicalContractKey(registryKey) + def, ok := manifest.Lookup(canonical) + if !ok { + continue // reported by the coverage test above + } + + t.Run(registryKey, func(t *testing.T) { + contractDefault, err := scalarDefault(def.DefaultValue) + if err != nil { + t.Skipf("contract default is not a scalar: %s", def.DefaultValue) + } + // The legacy registry stores every value as a string and has no way + // to say "unset", so it spells that as the empty string. The + // contract spells it null, which is why the language settings are + // nullable. Those are the same statement, not a disagreement. + if spec.DefaultValue == "" && string(def.DefaultValue) == "null" { + return + } + if spec.DefaultValue != contractDefault { + t.Errorf("default disagrees: settingsRegistry has %q, contract has %q", + spec.DefaultValue, contractDefault) + } + }) + } +} + +// scalarDefault renders a contract default the way the legacy registry would +// have stored it, so the two can be compared. +func scalarDefault(raw json.RawMessage) (string, error) { + var value any + if err := json.Unmarshal(raw, &value); err != nil { + return "", err + } + switch typed := value.(type) { + case string: + return typed, nil + case bool: + return strconv.FormatBool(typed), nil + case float64: + return strconv.FormatFloat(typed, 'f', -1, 64), nil + default: + return "", errNotScalar + } +} + +var errNotScalar = ¬ScalarError{} + +type notScalarError struct{} + +func (*notScalarError) Error() string { return "not a scalar default" } + +// TestContractLoadsUnderTheServerBuild is a cheap canary: the handlers package +// is linked into cmd/silo, so if the embedded manifest is self-inconsistent the +// failure shows up here rather than at a customer's startup. +func TestContractLoadsUnderTheServerBuild(t *testing.T) { + manifest, err := settingscontract.Load() + if err != nil { + t.Fatalf("embedded settings contract is invalid: %v", err) + } + if len(manifest.Keys()) == 0 { + t.Fatal("settings contract declares no keys") + } + for _, key := range manifest.Keys() { + if strings.TrimSpace(key) == "" { + t.Error("contract declares an empty key") + } + } +} diff --git a/internal/audiobooks/abs/bookmarks.go b/internal/audiobooks/abs/bookmarks.go index 4f3d7dc0..2ffad51a 100644 --- a/internal/audiobooks/abs/bookmarks.go +++ b/internal/audiobooks/abs/bookmarks.go @@ -31,7 +31,7 @@ type BookmarkStore interface { // the handlers use it. Intentionally narrow — only the fields the wire // format cares about. type Bookmark struct { - ID string // ULID + ID string // ULID LibraryItemID string Time float64 // fractional seconds Title string diff --git a/internal/audiobooks/abs/jwt.go b/internal/audiobooks/abs/jwt.go index 77c6d554..9549f3b9 100644 --- a/internal/audiobooks/abs/jwt.go +++ b/internal/audiobooks/abs/jwt.go @@ -12,10 +12,10 @@ import ( // Claims are the unified ABS JWT claim set. Different `Type` values denote // access, refresh, or session tokens. type Claims struct { - Type string `json:"type"` // access | refresh | session - UserID string `json:"sub"` // user id - ProfileID string `json:"pid,omitempty"` // empty = primary profile - JTI string `json:"jti"` // token id (revocable) + Type string `json:"type"` // access | refresh | session + UserID string `json:"sub"` // user id + ProfileID string `json:"pid,omitempty"` // empty = primary profile + JTI string `json:"jti"` // token id (revocable) DeviceID string `json:"device_id,omitempty"` SessionID string `json:"sid,omitempty"` BookID string `json:"bid,omitempty"` diff --git a/internal/audiobooks/abs_smart_collection_store.go b/internal/audiobooks/abs_smart_collection_store.go index 0f81a09b..bf3e65b3 100644 --- a/internal/audiobooks/abs_smart_collection_store.go +++ b/internal/audiobooks/abs_smart_collection_store.go @@ -22,7 +22,7 @@ import ( // collection_type = 'smart'. // // abs.SmartCollection.IsPublic maps to user_personal_collections.is_shared. -// profile_id is a text column (NOT NULL DEFAULT '') in the canonical +// profile_id is a text column (NOT NULL DEFAULT ”) in the canonical // schema, so the empty string stands in for "primary profile". // // abs.SmartCollection.Color and abs.SmartCollection.IsPinned have no diff --git a/internal/audiobooks/podcastfeed/refresher_test.go b/internal/audiobooks/podcastfeed/refresher_test.go index 9ad6ded1..ad798c82 100644 --- a/internal/audiobooks/podcastfeed/refresher_test.go +++ b/internal/audiobooks/podcastfeed/refresher_test.go @@ -17,8 +17,8 @@ import ( type fakeStore struct { mu sync.Mutex - feeds []podcastfeed.PodcastFeed - existingByGUID map[string]string + feeds []podcastfeed.PodcastFeed + existingByGUID map[string]string upsertedEpisodes []podcastfeed.PodcastEpisode refreshed map[string]string // media_item_id → last_error } diff --git a/internal/audiobooks/smartcoll/evaluator_test.go b/internal/audiobooks/smartcoll/evaluator_test.go index b7d449e7..578449ab 100644 --- a/internal/audiobooks/smartcoll/evaluator_test.go +++ b/internal/audiobooks/smartcoll/evaluator_test.go @@ -23,7 +23,7 @@ func TestEvaluate_EmptyRulesMatchesAll(t *testing.T) { func TestEvaluate_GenreContains(t *testing.T) { qd := QueryDefinition{ - Match: "all", + Match: "all", Groups: []QueryGroup{{Match: "all", Rules: []QueryRule{{Field: "genre", Op: "contains", Value: "Sci"}}}}, } got := Evaluate(context.Background(), qd, sampleCandidates(), EvaluateOptions{}) @@ -34,7 +34,7 @@ func TestEvaluate_GenreContains(t *testing.T) { func TestEvaluate_YearBetween(t *testing.T) { qd := QueryDefinition{ - Match: "all", + Match: "all", Groups: []QueryGroup{{Match: "all", Rules: []QueryRule{{Field: "year", Op: "between", Value: []any{2000, 2025}}}}}, } got := Evaluate(context.Background(), qd, sampleCandidates(), EvaluateOptions{}) @@ -45,7 +45,7 @@ func TestEvaluate_YearBetween(t *testing.T) { func TestEvaluate_AddedInLast14d(t *testing.T) { qd := QueryDefinition{ - Match: "all", + Match: "all", Groups: []QueryGroup{{Match: "all", Rules: []QueryRule{{Field: "added_at", Op: "in_last", Value: "14d"}}}}, } got := Evaluate(context.Background(), qd, sampleCandidates(), EvaluateOptions{Now: time.Now()}) @@ -72,7 +72,7 @@ func TestEvaluate_PersonalizedDroppedWithoutScope(t *testing.T) { cands := sampleCandidates() cands[0].IsFinished = true qd := QueryDefinition{ - Match: "all", + Match: "all", Groups: []QueryGroup{{Match: "all", Rules: []QueryRule{{Field: "finished", Op: "is", Value: true}}}}, } got := Evaluate(context.Background(), qd, cands, EvaluateOptions{AllowPersonalized: false}) @@ -91,7 +91,7 @@ func TestEvaluate_BookmarkCountGT(t *testing.T) { cands[1].BookmarkCount = 0 cands[2].BookmarkCount = 2 qd := QueryDefinition{ - Match: "all", + Match: "all", Groups: []QueryGroup{{Match: "all", Rules: []QueryRule{{Field: "bookmark_count", Op: "gt", Value: 0}}}}, } got := Evaluate(context.Background(), qd, cands, EvaluateOptions{AllowPersonalized: true}) @@ -133,7 +133,7 @@ func TestEvaluate_AbandonedRule(t *testing.T) { cands[0].CurrentSeconds = 1000 cands[0].LastPlayedAt = time.Now().Add(-90 * 24 * time.Hour) qd := QueryDefinition{ - Match: "all", + Match: "all", Groups: []QueryGroup{{Match: "all", Rules: []QueryRule{{Field: "abandoned", Op: "is", Value: true}}}}, } got := Evaluate(context.Background(), qd, cands, EvaluateOptions{AllowPersonalized: true, AbandonedAfter: 60 * 24 * time.Hour, Now: time.Now()}) diff --git a/internal/audiobooks/smartcoll/query_test.go b/internal/audiobooks/smartcoll/query_test.go index 1f6a24cc..8fe47285 100644 --- a/internal/audiobooks/smartcoll/query_test.go +++ b/internal/audiobooks/smartcoll/query_test.go @@ -12,7 +12,7 @@ func TestNormalize_DefaultsMatchToAll(t *testing.T) { func TestNormalize_LowercaseAndTrimsFields(t *testing.T) { q := QueryDefinition{ - Match: " ALL ", + Match: " ALL ", Groups: []QueryGroup{{Match: " Any ", Rules: []QueryRule{{Field: " Title ", Op: " IS ", Value: "x"}}}}, } n := q.Normalize() diff --git a/internal/models/library_collection.go b/internal/models/library_collection.go index b1e47295..b982f3cb 100644 --- a/internal/models/library_collection.go +++ b/internal/models/library_collection.go @@ -29,21 +29,21 @@ type LibraryCollection struct { // admin-uploaded posters (PosterAutoGenerated=false AND PosterFromTemplate=false) // remain sticky. PosterFromTemplate bool - SourceURL string - QueryDefinition json.RawMessage - SortConfig json.RawMessage - SourceConfig json.RawMessage - ManagementMode string - ManagementSource string - ManagementKey string - LastSyncStatus string - LastSyncMessage string - LastSyncAt *time.Time - SyncSchedule *string - NextSyncAt *time.Time - ItemCount int - CreatedAt time.Time - UpdatedAt time.Time + SourceURL string + QueryDefinition json.RawMessage + SortConfig json.RawMessage + SourceConfig json.RawMessage + ManagementMode string + ManagementSource string + ManagementKey string + LastSyncStatus string + LastSyncMessage string + LastSyncAt *time.Time + SyncSchedule *string + NextSyncAt *time.Time + ItemCount int + CreatedAt time.Time + UpdatedAt time.Time } type LibraryCollectionGroupKind string diff --git a/internal/models/marker_source.go b/internal/models/marker_source.go index 8b5cd7a7..5d4379a1 100644 --- a/internal/models/marker_source.go +++ b/internal/models/marker_source.go @@ -23,4 +23,3 @@ func MarkerSourcePriority(source string) int { return 0 } } - diff --git a/internal/settingscontract/canonical.go b/internal/settingscontract/canonical.go index 07f2e82d..a7f6c05e 100644 --- a/internal/settingscontract/canonical.go +++ b/internal/settingscontract/canonical.go @@ -9,8 +9,48 @@ import ( "math" "sort" "strconv" + "strings" + "sync" ) +// canonicalOnce guards the four derived representations below. All of them are +// pure functions of files embedded at compile time, so they are computed once +// and handed out as copies rather than rebuilt per request: a conditional GET +// of the manifest would otherwise pay a full parse and re-serialize just to +// answer 304. +var ( + canonicalOnce sync.Once + canonicalManifest []byte + canonicalPublic []byte + canonicalETag string + canonicalPublicTag string + canonicalErr error +) + +func computeCanonical() { + raw, err := RawBytes() + if err != nil { + canonicalErr = err + return + } + + canonicalManifest, canonicalErr = canonicalize(raw) + if canonicalErr != nil { + return + } + + canonicalPublic, canonicalErr = buildPublic(raw) + if canonicalErr != nil { + return + } + + canonicalETag, canonicalErr = digestWithSchemas(canonicalManifest) + if canonicalErr != nil { + return + } + canonicalPublicTag, canonicalErr = digestWithSchemas(canonicalPublic) +} + // CanonicalBytes returns the RFC 8785 (JCS) canonicalization of the manifest: // UTF-8, object keys sorted by code point, no insignificant whitespace. // @@ -18,22 +58,30 @@ import ( // what makes the ETag comparable across deployments and generated client code // reproducible. func CanonicalBytes() ([]byte, error) { - raw, err := RawBytes() - if err != nil { - return nil, err + canonicalOnce.Do(computeCanonical) + if canonicalErr != nil { + return nil, canonicalErr } - return canonicalize(raw) + return append([]byte(nil), canonicalManifest...), nil } -// ETag returns the SHA-256 digest of the canonical bytes, formatted as a strong -// HTTP entity tag. +// ETag returns the entity tag for the contract, formatted as a strong HTTP +// entity tag. +// +// The digest covers the value schemas under schemas/ as well as the manifest. +// Those files decide which object values the server accepts, so a change to one +// changes the contract even though manifest.json is byte-identical. Folding +// them in means the tag is a validator for the contract rather than for the +// served bytes alone: a client whose conditional GET misses re-reads a body +// that may not have changed, which is the cheap direction to be wrong in. The +// alternative — 304 forever while the server silently validates against a +// different schema — is the drift this contract exists to prevent. func ETag() (string, error) { - canonical, err := CanonicalBytes() - if err != nil { - return "", err + canonicalOnce.Do(computeCanonical) + if canonicalErr != nil { + return "", canonicalErr } - sum := sha256.Sum256(canonical) - return `"` + hex.EncodeToString(sum[:]) + `"`, nil + return canonicalETag, nil } // PublicBytes returns the canonicalized manifest with maintainer-only fields @@ -42,53 +90,94 @@ func ETag() (string, error) { // Only "notes" is stripped today. Internal storage bindings, when they exist, // are stripped here too: the public manifest must never name a table or column. func PublicBytes() ([]byte, error) { - raw, err := RawBytes() - if err != nil { - return nil, err + canonicalOnce.Do(computeCanonical) + if canonicalErr != nil { + return nil, canonicalErr } - - var doc map[string]any - decoder := json.NewDecoder(bytes.NewReader(raw)) - decoder.UseNumber() - if err := decoder.Decode(&doc); err != nil { - return nil, fmt.Errorf("parsing manifest: %w", err) - } - - definitions, _ := doc["definitions"].([]any) - for _, entry := range definitions { - def, ok := entry.(map[string]any) - if !ok { - continue - } - delete(def, "notes") - } - - encoded, err := json.Marshal(doc) - if err != nil { - return nil, fmt.Errorf("encoding public manifest: %w", err) - } - return canonicalize(encoded) + return append([]byte(nil), canonicalPublic...), nil } // PublicETag returns the entity tag for the public manifest projection. It // differs from ETag because stripping notes changes the bytes clients see. func PublicETag() (string, error) { - public, err := PublicBytes() + canonicalOnce.Do(computeCanonical) + if canonicalErr != nil { + return "", canonicalErr + } + return canonicalPublicTag, nil +} + +// buildPublic strips maintainer-only fields and canonicalizes in one pass. The +// decoded document is handed straight to the canonical writer, which already +// understands map[string]any / []any / json.Number, rather than being +// re-marshalled and re-parsed. +func buildPublic(raw []byte) ([]byte, error) { + doc, err := decodeJSON(raw) + if err != nil { + return nil, err + } + + root, _ := doc.(map[string]any) + definitions, _ := root["definitions"].([]any) + for _, entry := range definitions { + if def, ok := entry.(map[string]any); ok { + delete(def, "notes") + } + } + + return canonicalizeValue(doc) +} + +// digestWithSchemas hashes a canonical manifest projection together with every +// value schema, keyed by filename so a rename is a change too. +func digestWithSchemas(manifest []byte) (string, error) { + schemas, err := SchemaBytes() if err != nil { return "", err } - sum := sha256.Sum256(public) - return `"` + hex.EncodeToString(sum[:]) + `"`, nil + + names := make([]string, 0, len(schemas)) + for name := range schemas { + names = append(names, name) + } + sort.Strings(names) + + digest := sha256.New() + digest.Write(manifest) + for _, name := range names { + canonical, err := canonicalize(schemas[name]) + if err != nil { + return "", fmt.Errorf("canonicalizing value schema %s: %w", name, err) + } + // Length-prefixed so no combination of names and bodies can collide + // with a different combination. + fmt.Fprintf(digest, "\n%d:%s\n%d:", len(name), name, len(canonical)) + digest.Write(canonical) + } + + sum := digest.Sum(nil) + return `"` + hex.EncodeToString(sum) + `"`, nil } -func canonicalize(raw []byte) ([]byte, error) { +func decodeJSON(raw []byte) (any, error) { var doc any decoder := json.NewDecoder(bytes.NewReader(raw)) decoder.UseNumber() if err := decoder.Decode(&doc); err != nil { return nil, fmt.Errorf("parsing JSON for canonicalization: %w", err) } + return doc, nil +} +func canonicalize(raw []byte) ([]byte, error) { + doc, err := decodeJSON(raw) + if err != nil { + return nil, err + } + return canonicalizeValue(doc) +} + +func canonicalizeValue(doc any) ([]byte, error) { var out bytes.Buffer if err := writeCanonical(&out, doc); err != nil { return nil, err @@ -116,11 +205,7 @@ func writeCanonical(out *bytes.Buffer, value any) error { out.WriteString(serialized) case string: - encoded, err := json.Marshal(typed) - if err != nil { - return fmt.Errorf("encoding string: %w", err) - } - out.Write(encoded) + writeCanonicalString(out, typed) case []any: out.WriteByte('[') @@ -148,11 +233,7 @@ func writeCanonical(out *bytes.Buffer, value any) error { if i > 0 { out.WriteByte(',') } - encodedKey, err := json.Marshal(key) - if err != nil { - return fmt.Errorf("encoding key %q: %w", key, err) - } - out.Write(encodedKey) + writeCanonicalString(out, key) out.WriteByte(':') if err := writeCanonical(out, typed[key]); err != nil { return err @@ -167,6 +248,43 @@ func writeCanonical(out *bytes.Buffer, value any) error { return nil } +// writeCanonicalString escapes a string the way ECMAScript's JSON.stringify +// does, which is what JCS requires. +// +// encoding/json is deliberately not used here: json.Marshal HTML-escapes <, > +// and &, which JCS emits literally. Nothing in the manifest triggers that +// today, so the difference would first appear as an ETag that silently +// disagrees with every conforming client the moment a label contains an +// ampersand. +func writeCanonicalString(out *bytes.Buffer, value string) { + out.WriteByte('"') + for _, r := range value { + switch r { + case '"': + out.WriteString(`\"`) + case '\\': + out.WriteString(`\\`) + case '\b': + out.WriteString(`\b`) + case '\f': + out.WriteString(`\f`) + case '\n': + out.WriteString(`\n`) + case '\r': + out.WriteString(`\r`) + case '\t': + out.WriteString(`\t`) + default: + if r < 0x20 { + fmt.Fprintf(out, `\u%04x`, r) + continue + } + out.WriteRune(r) + } + } + out.WriteByte('"') +} + // canonicalNumber renders a number the way ECMAScript's Number::toString does, // which is what JCS requires: 3.0 becomes "3", 0.05 stays "0.05". func canonicalNumber(number json.Number) (string, error) { @@ -177,8 +295,60 @@ func canonicalNumber(number json.Number) (string, error) { if math.IsNaN(parsed) || math.IsInf(parsed, 0) { return "", fmt.Errorf("NaN and infinities cannot be canonicalized: %s", number) } - if parsed == math.Trunc(parsed) && math.Abs(parsed) < 1e21 { - return strconv.FormatFloat(parsed, 'f', 0, 64), nil - } - return strconv.FormatFloat(parsed, 'g', -1, 64), nil + return formatECMAScript(parsed) +} + +// formatECMAScript implements ECMA-262 Number::toString for radix 10. +// +// strconv is used only for the shortest round-tripping digits and the decimal +// exponent; the layout is reassembled here because Go's own formats differ from +// ECMAScript in three ways that all silently change the digest: 'g' switches to +// exponential at 1e-5 where ECMAScript switches at 1e-7, Go zero-pads the +// exponent ("1e-07" vs "1e-7"), and Go prints negative zero as "-0" where +// ECMAScript and JCS print "0". +func formatECMAScript(value float64) (string, error) { + if value == 0 { + return "0", nil + } + + sign := "" + if value < 0 { + sign = "-" + value = -value + } + + // "d.dddde±dd" with the fewest digits that round-trip. + scientific := strconv.FormatFloat(value, 'e', -1, 64) + separator := strings.IndexByte(scientific, 'e') + if separator < 0 { + return "", fmt.Errorf("unexpected float formatting for %v", value) + } + exponent, err := strconv.Atoi(scientific[separator+1:]) + if err != nil { + return "", fmt.Errorf("parsing exponent of %v: %w", value, err) + } + digits := strings.Replace(scientific[:separator], ".", "", 1) + + // ECMA-262 names these k (digit count) and n (position of the decimal + // point relative to the digits), where the value is digits × 10^(n-k). + k := len(digits) + n := exponent + 1 + + switch { + case k <= n && n <= 21: + return sign + digits + strings.Repeat("0", n-k), nil + case 0 < n && n <= 21: + return sign + digits[:n] + "." + digits[n:], nil + case -6 < n && n <= 0: + return sign + "0." + strings.Repeat("0", -n) + digits, nil + } + + suffix := "e+" + strconv.Itoa(n-1) + if n-1 < 0 { + suffix = "e-" + strconv.Itoa(1-n) + } + if k == 1 { + return sign + digits + suffix, nil + } + return sign + digits[:1] + "." + digits[1:] + suffix, nil } diff --git a/internal/settingscontract/contract.go b/internal/settingscontract/contract.go index 37c4b3c8..96d670b9 100644 --- a/internal/settingscontract/contract.go +++ b/internal/settingscontract/contract.go @@ -13,6 +13,7 @@ package settingscontract import ( "encoding/json" "fmt" + "regexp" ) // Scope identifies the storage identity a value is attached to. @@ -186,6 +187,10 @@ type ValueSchema struct { // Object. SchemaRef string `json:"schema_ref,omitempty"` + + // compiledPattern is Pattern, compiled once when the manifest loads. + // ValidateValue runs per request, so it must not compile a regex per call. + compiledPattern *regexp.Regexp } // EnumMember is one allowed enum value. Members are objects rather than bare diff --git a/internal/settingscontract/contract_test.go b/internal/settingscontract/contract_test.go index 20d0303d..9647ecd0 100644 --- a/internal/settingscontract/contract_test.go +++ b/internal/settingscontract/contract_test.go @@ -1,7 +1,11 @@ package settingscontract import ( + "crypto/sha256" + "encoding/hex" "encoding/json" + "fmt" + "sort" "strings" "testing" ) @@ -311,6 +315,224 @@ func TestCanonicalizationSortsKeysAndNormalizesNumbers(t *testing.T) { } } +// TestNumbersMatchECMAScript pins the cases where Go's own float formatting +// disagrees with ECMAScript's Number::toString, which is what RFC 8785 +// requires. Every expectation below is the literal output of String(x) in +// node; a client running a conforming JCS library must agree byte for byte or +// its digest of the same manifest will differ from the server's. +func TestNumbersMatchECMAScript(t *testing.T) { + cases := []struct{ in, want string }{ + // Go's 'g' would switch to exponential at 1e-5; ECMAScript holds off + // until the exponent drops below -6. + {"0.00001", "0.00001"}, + {"1e-5", "0.00001"}, + {"0.000001", "0.000001"}, + {"1e-6", "0.000001"}, + // Below the threshold both use exponential, but Go zero-pads. + {"1e-7", "1e-7"}, + {"-1e-7", "-1e-7"}, + {"5e-324", "5e-324"}, + // Go prints negative zero as "-0"; JCS has no negative zero. + {"-0", "0"}, + {"0", "0"}, + // Integers render without a fraction up to 1e21, exponential above. + {"1e20", "100000000000000000000"}, + {"1e21", "1e+21"}, + {"1e22", "1e+22"}, + {"1e100", "1e+100"}, + {"3.0", "3"}, + {"1e2", "100"}, + // Shortest round-trip digits, not the exact binary value. + {"1234567.5", "1234567.5"}, + {"0.1", "0.1"}, + {"2.50", "2.5"}, + {"-2.5", "-2.5"}, + {"1.5e300", "1.5e+300"}, + {"123.456", "123.456"}, + } + + for _, tc := range cases { + got, err := canonicalize([]byte(tc.in)) + if err != nil { + t.Errorf("canonicalize(%s): %v", tc.in, err) + continue + } + if string(got) != tc.want { + t.Errorf("canonicalize(%s) = %s, want %s (ECMAScript String())", tc.in, got, tc.want) + } + } +} + +// TestStringsAreNotHTMLEscaped guards the other half of JCS string handling. +// encoding/json escapes <, > and & by default, which would silently fork the +// server's digest from every conforming client the first time a label contains +// an ampersand. +func TestStringsAreNotHTMLEscaped(t *testing.T) { + got, err := canonicalize([]byte(`{"label":"Audio & subtitles","hint":" or >90%"}`)) + if err != nil { + t.Fatalf("canonicalizing: %v", err) + } + want := `{"hint":" or >90%","label":"Audio & subtitles"}` + if string(got) != want { + t.Fatalf("canonicalize() = %s, want %s", got, want) + } + + // The escapes JCS does require are still applied. + got, err = canonicalize([]byte(`{"a":"q\"uote\\back\ttab\nline"}`)) + if err != nil { + t.Fatalf("canonicalizing escapes: %v", err) + } + if want := `{"a":"q\"uote\\back\ttab\nline"}`; string(got) != want { + t.Fatalf("canonicalize() = %s, want %s", got, want) + } + + // Control characters below 0x20 with no short escape use \u00xx. + got, err = canonicalize([]byte(`{"a":"\u0001"}`)) + if err != nil { + t.Fatalf("canonicalizing control character: %v", err) + } + if want := `{"a":"\u0001"}`; string(got) != want { + t.Fatalf("canonicalize() = %s, want %s", got, want) + } +} + +// TestETagCoversValueSchemas fails if the entity tag is ever narrowed back to +// manifest.json alone. The schemas decide which object values the server +// accepts, so a change to one changes the contract; if it did not move the tag, +// every client would 304 forever against validation rules that had shifted +// underneath them. +func TestETagCoversValueSchemas(t *testing.T) { + schemas, err := SchemaBytes() + if err != nil { + t.Fatalf("reading value schemas: %v", err) + } + if len(schemas) == 0 { + t.Fatal("no value schemas, so this test proves nothing") + } + + manifest, err := CanonicalBytes() + if err != nil { + t.Fatalf("canonicalizing manifest: %v", err) + } + baseline, err := digestWithSchemas(manifest) + if err != nil { + t.Fatalf("digesting: %v", err) + } + + live, err := ETag() + if err != nil { + t.Fatalf("computing ETag: %v", err) + } + if live != baseline { + t.Fatalf("ETag() = %s, want the schema-inclusive digest %s", live, baseline) + } + + if reference := sha256Of(manifest, schemas); reference != baseline { + t.Fatalf("test digest helper disagrees with digestWithSchemas: %s vs %s", + reference, baseline) + } + + // Tightening a schema — the change that alters what the server accepts + // while leaving manifest.json byte-identical — must move the digest. + names := make([]string, 0, len(schemas)) + for name := range schemas { + names = append(names, name) + } + sort.Strings(names) + + tightened := map[string][]byte{} + for name, body := range schemas { + tightened[name] = body + } + tightened[names[0]] = []byte(`{"type":"object","additionalProperties":false}`) + if sha256Of(manifest, tightened) == baseline { + t.Error("changing a value schema's contents did not change the contract digest") + } + + // So must renaming one, since the filename is what a definition's + // schema_ref binds to. + renamed := map[string][]byte{} + for name, body := range schemas { + renamed[name] = body + } + renamed["renamed-"+names[0]] = renamed[names[0]] + delete(renamed, names[0]) + if sha256Of(manifest, renamed) == baseline { + t.Error("renaming a value schema did not change the contract digest") + } + + // Whitespace, on the other hand, must not: the schemas are canonicalized + // before hashing, so reformatting a file is not a contract change. + reformatted := map[string][]byte{} + for name, body := range schemas { + reformatted[name] = body + } + reformatted[names[0]] = append(append([]byte(nil), reformatted[names[0]]...), '\n', ' ') + if sha256Of(manifest, reformatted) != baseline { + t.Error("reformatting a value schema changed the contract digest") + } +} + +func sha256Of(manifest []byte, schemas map[string][]byte) string { + names := make([]string, 0, len(schemas)) + for name := range schemas { + names = append(names, name) + } + sort.Strings(names) + + digest := sha256.New() + digest.Write(manifest) + for _, name := range names { + canonical, err := canonicalize(schemas[name]) + if err != nil { + // A deliberately-perturbed schema may not parse; hash the raw bytes + // so the test still observes a change. + canonical = schemas[name] + } + fmt.Fprintf(digest, "\n%d:%s\n%d:", len(name), name, len(canonical)) + digest.Write(canonical) + } + return `"` + hex.EncodeToString(digest.Sum(nil)) + `"` +} + +// TestDerivedRepresentationsAreMemoized keeps the manifest endpoint cheap: all +// four values are pure functions of files fixed at compile time, and a +// conditional GET must not pay a full parse and re-serialize to answer 304. +func TestDerivedRepresentationsAreMemoized(t *testing.T) { + first, err := CanonicalBytes() + if err != nil { + t.Fatalf("canonicalizing: %v", err) + } + second, err := CanonicalBytes() + if err != nil { + t.Fatalf("canonicalizing again: %v", err) + } + + // Callers get their own copy, so mutating one must not corrupt the cache. + if len(first) > 0 { + first[0] = 'X' + } + third, err := CanonicalBytes() + if err != nil { + t.Fatalf("canonicalizing a third time: %v", err) + } + if string(second) != string(third) { + t.Fatal("mutating a returned slice corrupted the memoized canonical bytes") + } + + allocs := testing.AllocsPerRun(100, func() { + if _, err := ETag(); err != nil { + t.Fatalf("computing ETag: %v", err) + } + if _, err := PublicETag(); err != nil { + t.Fatalf("computing PublicETag: %v", err) + } + }) + if allocs > 0 { + t.Errorf("ETag()+PublicETag() allocate %.0f times per call; both should be memoized", allocs) + } +} + func TestPublicManifestStripsMaintainerNotes(t *testing.T) { public, err := PublicBytes() if err != nil { @@ -568,3 +790,162 @@ func TestDuplicateKeysAreRejected(t *testing.T) { t.Fatal("index accepted a duplicate key") } } + +// TestStrictUnmarshalRejectsTrailingContent covers the bytes a caller slicing a +// value out of a larger document is most likely to hand over. json.Decoder's +// More() answers false for a stray closing bracket, so relying on it let +// `true]` validate as a boolean. +func TestStrictUnmarshalRejectsTrailingContent(t *testing.T) { + rejected := []string{ + `true]`, `true}`, `30]`, `30}`, `"always"}`, `"a" ]`, + `true false`, `30 40`, `{} {}`, `[] []`, + } + for _, raw := range rejected { + var value any + if err := strictUnmarshal([]byte(raw), &value); err == nil { + t.Errorf("strictUnmarshal(%s) accepted trailing content", raw) + } + } + + accepted := []string{`true`, `30`, `"always"`, `{"a":1}`, `[1,2]`, `null`, ` 30 `} + for _, raw := range accepted { + var value any + if err := strictUnmarshal([]byte(raw), &value); err != nil { + t.Errorf("strictUnmarshal(%s) = %v, want nil", raw, err) + } + } +} + +// TestEnumMatchingIsTypeSafe guards the value types manifest.schema.json +// already permits. Comparing formatted tokens made the string "3" satisfy an +// integer member and "true" satisfy a boolean one. +func TestEnumMatchingIsTypeSafe(t *testing.T) { + schema := &ValueSchema{ + Type: TypeEnum, + Values: []EnumMember{ + {Value: "auto"}, + {Value: float64(3)}, + {Value: true}, + {Value: float64(1000000)}, + }, + } + + accepted := []string{`"auto"`, `3`, `true`, `3.0`, `1000000`, `1e6`} + for _, raw := range accepted { + if err := schema.ValidateValue(json.RawMessage(raw), nil); err != nil { + t.Errorf("ValidateValue(%s) = %v, want nil", raw, err) + } + } + + rejected := []string{`"3"`, `"true"`, `"1000000"`, `4`, `false`, `"AUTO"`} + for _, raw := range rejected { + if err := schema.ValidateValue(json.RawMessage(raw), nil); err == nil { + t.Errorf("ValidateValue(%s) was accepted; wrong JSON type or value", raw) + } + } + + // A string member and a numeric member that print alike are distinct, not + // a duplicate. + mixed := &ValueSchema{ + Type: TypeEnum, + Values: []EnumMember{{Value: "3"}, {Value: float64(3)}}, + } + if errs := mixed.validate(1, nil); len(errs) != 0 { + t.Errorf(`members "3" and 3 reported as duplicates: %v`, errs) + } +} + +// TestStepIsEnforced closes the gap between a declared constraint and the +// single validation path. player.playback_speed advertises a 0.05 step, so a +// server that stores 1.4372 hands every client a value its stepper cannot +// represent. +func TestStepIsEnforced(t *testing.T) { + min, max, step := 0.25, 3.0, 0.05 + schema := &ValueSchema{ + Type: TypeNumber, Minimum: &min, Maximum: &max, Step: &step, + } + + for _, raw := range []string{`0.25`, `0.75`, `1`, `1.25`, `1.4`, `2.5`, `3`} { + if err := schema.ValidateValue(json.RawMessage(raw), nil); err != nil { + t.Errorf("ValidateValue(%s) = %v, want nil", raw, err) + } + } + for _, raw := range []string{`0.26`, `1.4372`, `1.01`, `2.99`} { + if err := schema.ValidateValue(json.RawMessage(raw), nil); err == nil { + t.Errorf("ValidateValue(%s) was accepted despite the 0.05 step", raw) + } + } + + // Range still wins where both apply. + if err := schema.ValidateValue(json.RawMessage(`3.05`), nil); err == nil { + t.Error("a value above the maximum was accepted") + } +} + +// TestLanguageTagsAcceptWhatClientsProduce pins the shapes the narrower +// original pattern rejected. Every tag here is one a shipped client can emit +// without the user doing anything unusual. +func TestLanguageTagsAcceptWhatClientsProduce(t *testing.T) { + wellFormed := map[string]string{ + "en": "en", + "EN": "en", + "en-US": "en-US", + "en-us": "en-US", + "EN-us": "en-US", + "en_US": "en-US", // iOS Locale.identifier, Android Locale.toString() + "zh-Hant-TW": "zh-Hant-TW", + "zh-hant-tw": "zh-Hant-TW", + "ca-ES-valencia": "ca-ES-valencia", + "ar-EG-u-nu-latn": "ar-EG-u-nu-latn", + "de-DE-u-co-phonebk": "de-DE-u-co-phonebk", + "es-419": "es-419", + "en-US-x-private": "en-US-x-private", + } + for input, want := range wellFormed { + got, ok := NormalizeLanguageTag(input) + if !ok { + t.Errorf("NormalizeLanguageTag(%q) rejected a tag a client produces", input) + continue + } + if got != want { + t.Errorf("NormalizeLanguageTag(%q) = %q, want %q", input, got, want) + } + } + + // The empty string is not a language tag: "no preference" is null, which + // the nullable flag on each language setting expresses. + malformed := []string{"", " ", "e", "toolongprimary", "en-", "-US", "en--US", "123"} + for _, input := range malformed { + if got, ok := NormalizeLanguageTag(input); ok { + t.Errorf("NormalizeLanguageTag(%q) = %q, want rejection", input, got) + } + } +} + +// TestNormalizeValueCanonicalizesLanguageTags proves normalization is reachable +// through the shared path, not just available as a helper. Without it en-US, +// en-us and EN-us are three rows for one preference and track matching misses +// on two of them. +func TestNormalizeValueCanonicalizesLanguageTags(t *testing.T) { + schema := &ValueSchema{Type: TypeLanguageTag, Nullable: true} + + got, err := schema.NormalizeValue(json.RawMessage(`"en_us"`), nil) + if err != nil { + t.Fatalf("NormalizeValue: %v", err) + } + if string(got) != `"en-US"` { + t.Fatalf("NormalizeValue = %s, want \"en-US\"", got) + } + + got, err = schema.NormalizeValue(json.RawMessage(`null`), nil) + if err != nil { + t.Fatalf("NormalizeValue(null): %v", err) + } + if string(got) != jsonNull { + t.Fatalf("NormalizeValue(null) = %s, want null", got) + } + + if _, err := schema.NormalizeValue(json.RawMessage(`""`), nil); err == nil { + t.Error(`NormalizeValue("") was accepted; unset must be null, not the empty string`) + } +} diff --git a/internal/settingscontract/load.go b/internal/settingscontract/load.go index 878deb5a..1d4835f8 100644 --- a/internal/settingscontract/load.go +++ b/internal/settingscontract/load.go @@ -24,13 +24,24 @@ const ( ) var ( - loadOnce sync.Once - loaded *Manifest - loadedErr error - loadedRaw []byte - objSchemas map[string]*jsonschema.Schema + loadOnce sync.Once + loaded *Manifest + loadedErr error + loadedRaw []byte + loadedSchemas map[string][]byte + objSchemas map[string]*jsonschema.Schema ) +// loaded contract, as returned by load(). Keeping this a value rather than +// having load() assign the package globals means load() stays pure and can be +// called with a test filesystem without clobbering the process-wide contract. +type contract struct { + manifest *Manifest + raw []byte + schemaRaw map[string][]byte + compiled map[string]*jsonschema.Schema +} + // Load returns the embedded canonical manifest, parsed and fully validated. // // It is loaded once per process. A malformed or self-inconsistent manifest is a @@ -39,7 +50,15 @@ var ( // rather than degrading. func Load() (*Manifest, error) { loadOnce.Do(func() { - loaded, loadedRaw, loadedErr = load(contractFS) + result, err := load(contractFS) + if err != nil { + loadedErr = err + return + } + loaded = result.manifest + loadedRaw = result.raw + loadedSchemas = result.schemaRaw + objSchemas = result.compiled }) return loaded, loadedErr } @@ -62,6 +81,20 @@ func RawBytes() ([]byte, error) { return append([]byte(nil), loadedRaw...), nil } +// SchemaBytes returns every value schema file exactly as checked in, keyed by +// file name. These decide which object-typed values the contract accepts, so +// they are part of its identity — see ETag. +func SchemaBytes() (map[string][]byte, error) { + if _, err := Load(); err != nil { + return nil, err + } + out := make(map[string][]byte, len(loadedSchemas)) + for name, body := range loadedSchemas { + out[name] = append([]byte(nil), body...) + } + return out, nil +} + // ObjectSchema returns the compiled JSON Schema for an object-typed value. func ObjectSchema(ref string) (*jsonschema.Schema, bool) { if _, err := Load(); err != nil { @@ -71,38 +104,37 @@ func ObjectSchema(ref string) (*jsonschema.Schema, bool) { return schema, ok } -func load(fsys fs.FS) (*Manifest, []byte, error) { +func load(fsys fs.FS) (contract, error) { raw, err := fs.ReadFile(fsys, manifestPath) if err != nil { - return nil, nil, fmt.Errorf("reading manifest: %w", err) + return contract{}, fmt.Errorf("reading manifest: %w", err) } if err := validateAgainstManifestSchema(fsys, raw); err != nil { - return nil, nil, err + return contract{}, err } var manifest Manifest decoder := json.NewDecoder(bytes.NewReader(raw)) decoder.DisallowUnknownFields() if err := decoder.Decode(&manifest); err != nil { - return nil, nil, fmt.Errorf("parsing manifest: %w", err) + return contract{}, fmt.Errorf("parsing manifest: %w", err) } if err := manifest.index(); err != nil { - return nil, nil, err + return contract{}, err } - schemas, err := compileObjectSchemas(fsys) + schemaRaw, compiled, err := compileObjectSchemas(fsys) if err != nil { - return nil, nil, err - } - objSchemas = schemas - - if err := manifest.Validate(schemas); err != nil { - return nil, nil, err + return contract{}, err } - return &manifest, raw, nil + if err := manifest.Validate(compiled); err != nil { + return contract{}, err + } + + return contract{manifest: &manifest, raw: raw, schemaRaw: schemaRaw, compiled: compiled}, nil } // validateAgainstManifestSchema checks the manifest file against its own JSON @@ -142,13 +174,14 @@ func validateAgainstManifestSchema(fsys fs.FS, raw []byte) error { // values and their defaults can be validated. Compiling all of them up front // also catches a malformed schema file that no definition happens to reference // yet. -func compileObjectSchemas(fsys fs.FS) (map[string]*jsonschema.Schema, error) { +func compileObjectSchemas(fsys fs.FS) (map[string][]byte, map[string]*jsonschema.Schema, error) { entries, err := fs.ReadDir(fsys, schemasDir) if err != nil { - return nil, fmt.Errorf("reading value schema directory: %w", err) + return nil, nil, fmt.Errorf("reading value schema directory: %w", err) } compiler := jsonschema.NewCompiler() + raw := make(map[string][]byte, len(entries)) names := make([]string, 0, len(entries)) for _, entry := range entries { if entry.IsDir() { @@ -157,15 +190,16 @@ func compileObjectSchemas(fsys fs.FS) (map[string]*jsonschema.Schema, error) { name := entry.Name() body, err := fs.ReadFile(fsys, path.Join(schemasDir, name)) if err != nil { - return nil, fmt.Errorf("reading value schema %s: %w", name, err) + return nil, nil, fmt.Errorf("reading value schema %s: %w", name, err) } doc, err := jsonschema.UnmarshalJSON(bytes.NewReader(body)) if err != nil { - return nil, fmt.Errorf("parsing value schema %s: %w", name, err) + return nil, nil, fmt.Errorf("parsing value schema %s: %w", name, err) } if err := compiler.AddResource(name, doc); err != nil { - return nil, fmt.Errorf("registering value schema %s: %w", name, err) + return nil, nil, fmt.Errorf("registering value schema %s: %w", name, err) } + raw[name] = body names = append(names, name) } @@ -173,9 +207,9 @@ func compileObjectSchemas(fsys fs.FS) (map[string]*jsonschema.Schema, error) { for _, name := range names { schema, err := compiler.Compile(name) if err != nil { - return nil, fmt.Errorf("compiling value schema %s: %w", name, err) + return nil, nil, fmt.Errorf("compiling value schema %s: %w", name, err) } compiled[name] = schema } - return compiled, nil + return raw, compiled, nil } diff --git a/internal/settingscontract/validate.go b/internal/settingscontract/validate.go index ed1f7ea1..096aa07b 100644 --- a/internal/settingscontract/validate.go +++ b/internal/settingscontract/validate.go @@ -5,7 +5,10 @@ import ( "encoding/json" "errors" "fmt" + "io" + "math" "regexp" + "strconv" "strings" "github.com/santhosh-tekuri/jsonschema/v6" @@ -172,13 +175,20 @@ func (v *ValueSchema) validate(manifestRevision int, objectSchemas map[string]*j if v.Step != nil && *v.Step <= 0 { errs = append(errs, fmt.Errorf("step must be positive, got %g", *v.Step)) } - for label, rev := range map[string]int{ - "minimum_introduced_in": v.MinimumIntroducedIn, - "maximum_introduced_in": v.MaximumIntroducedIn, + // A slice, not a map: ranging a map would order these two errors + // randomly, so the same broken manifest would report differently from + // run to run. + for _, tagged := range []struct { + label string + rev int + }{ + {"minimum_introduced_in", v.MinimumIntroducedIn}, + {"maximum_introduced_in", v.MaximumIntroducedIn}, } { - if rev != 0 && rev > manifestRevision { + if tagged.rev != 0 && tagged.rev > manifestRevision { errs = append(errs, fmt.Errorf( - "%s is %d, after the manifest revision %d", label, rev, manifestRevision)) + "%s is %d, after the manifest revision %d", + tagged.label, tagged.rev, manifestRevision)) } } @@ -193,8 +203,15 @@ func (v *ValueSchema) validate(manifestRevision int, objectSchemas map[string]*j "min_length %d exceeds max_length %d", *v.MinLength, *v.MaxLength)) } if v.Pattern != "" { - if _, err := regexp.Compile(v.Pattern); err != nil { + // Compiled once here and reused by every ValidateValue call. + // ValidateValue is documented as the per-request validation path, + // so recompiling the pattern on each call would put a regex + // compile on every settings write. + compiled, err := regexp.Compile(v.Pattern) + if err != nil { errs = append(errs, fmt.Errorf("pattern does not compile: %w", err)) + } else { + v.compiledPattern = compiled } } @@ -204,15 +221,17 @@ func (v *ValueSchema) validate(manifestRevision int, objectSchemas map[string]*j } seen := make(map[string]struct{}, len(v.Values)) for _, member := range v.Values { - token := fmt.Sprintf("%v", member.Value) + // Type-tagged, so a string member "3" and an integer member 3 are + // two distinct members rather than a reported duplicate. + token := enumToken(member.Value) if _, dup := seen[token]; dup { - errs = append(errs, fmt.Errorf("enum repeats value %q", token)) + errs = append(errs, fmt.Errorf("enum repeats value %s", displayEnumValue(member.Value))) } seen[token] = struct{}{} if member.IntroducedIn != 0 && member.IntroducedIn > manifestRevision { errs = append(errs, fmt.Errorf( - "enum member %q claims introduced_in %d, after the manifest revision %d", - token, member.IntroducedIn, manifestRevision)) + "enum member %s claims introduced_in %d, after the manifest revision %d", + displayEnumValue(member.Value), member.IntroducedIn, manifestRevision)) } } @@ -338,20 +357,19 @@ func (v *ValueSchema) ValidateValue(raw json.RawMessage, objectSchemas map[strin if err := strictUnmarshal(trimmed, &value); err != nil { return fmt.Errorf("expected an enum value: %w", err) } - token := fmt.Sprintf("%v", value) for _, member := range v.Values { - if fmt.Sprintf("%v", member.Value) == token { + if enumMatches(value, member.Value) { return nil } } - return fmt.Errorf("%q is not one of %s", token, v.enumTokens()) + return fmt.Errorf("%s is not one of %s", displayEnumValue(value), v.enumTokens()) case TypeLanguageTag: var value string if err := strictUnmarshal(trimmed, &value); err != nil { return fmt.Errorf("expected a language tag: %w", err) } - if !languageTagPattern.MatchString(value) { + if _, ok := NormalizeLanguageTag(value); !ok { return fmt.Errorf("%q is not a well-formed BCP 47 language tag", value) } @@ -375,6 +393,45 @@ func (v *ValueSchema) ValidateValue(raw json.RawMessage, objectSchemas map[strin return nil } +// NormalizeValue validates a value and returns the form that should be stored. +// +// Anything that persists a value goes through here rather than ValidateValue, +// so the row that lands in the database is the one every client compares +// against. Only language tags differ from their input today; every other type +// is already canonical once it validates. +func (v *ValueSchema) NormalizeValue( + raw json.RawMessage, + objectSchemas map[string]*jsonschema.Schema, +) (json.RawMessage, error) { + if err := v.ValidateValue(raw, objectSchemas); err != nil { + return nil, err + } + + trimmed := bytes.TrimSpace(raw) + if v.Type != TypeLanguageTag || bytes.Equal(trimmed, []byte("null")) { + return append(json.RawMessage(nil), trimmed...), nil + } + + var tag string + if err := strictUnmarshal(trimmed, &tag); err != nil { + return nil, fmt.Errorf("expected a language tag: %w", err) + } + normalized, ok := NormalizeLanguageTag(tag) + if !ok { + return nil, fmt.Errorf("%q is not a well-formed BCP 47 language tag", tag) + } + encoded, err := json.Marshal(normalized) + if err != nil { + return nil, fmt.Errorf("encoding normalized language tag: %w", err) + } + return encoded, nil +} + +// stepTolerance absorbs binary floating-point error when checking a value +// against a declared step. 0.05 is not exactly representable, so requiring an +// exact multiple would reject values every client can legitimately produce. +const stepTolerance = 1e-9 + func (v *ValueSchema) checkRange(value float64) error { if v.Minimum != nil && value < *v.Minimum { return fmt.Errorf("%g is below the minimum %g", value, *v.Minimum) @@ -382,6 +439,19 @@ func (v *ValueSchema) checkRange(value float64) error { if v.Maximum != nil && value > *v.Maximum { return fmt.Errorf("%g is above the maximum %g", value, *v.Maximum) } + if v.Step != nil && *v.Step > 0 { + // Steps are counted from the minimum, which is the only origin every + // client's stepper agrees on. + base := 0.0 + if v.Minimum != nil { + base = *v.Minimum + } + steps := (value - base) / *v.Step + if math.Abs(steps-math.Round(steps))**v.Step > stepTolerance { + return fmt.Errorf("%g is not a multiple of the step %g from %g", + value, *v.Step, base) + } + } return nil } @@ -394,9 +464,15 @@ func (v *ValueSchema) checkString(value string) error { return fmt.Errorf("is longer than the maximum %d characters", *v.MaxLength) } if v.Pattern != "" { - matcher, err := regexp.Compile(v.Pattern) - if err != nil { - return fmt.Errorf("pattern does not compile: %w", err) + matcher := v.compiledPattern + if matcher == nil { + // Only reachable for a schema built in a test rather than loaded + // from the manifest, where validate() would have compiled it. + compiled, err := regexp.Compile(v.Pattern) + if err != nil { + return fmt.Errorf("pattern does not compile: %w", err) + } + matcher = compiled } if !matcher.MatchString(value) { return fmt.Errorf("does not match %s", v.Pattern) @@ -408,11 +484,92 @@ func (v *ValueSchema) checkString(value string) error { func (v *ValueSchema) enumTokens() string { tokens := make([]string, 0, len(v.Values)) for _, member := range v.Values { - tokens = append(tokens, fmt.Sprintf("%v", member.Value)) + tokens = append(tokens, displayEnumValue(member.Value)) } return strings.Join(tokens, ", ") } +// enumMatches reports whether a decoded request value is the same JSON value as +// an enum member. +// +// Comparison is by JSON type and value rather than by formatted text. +// manifest.schema.json permits string, integer and boolean members, and +// comparing "%v" tokens would let the string "3" satisfy an integer member 3 +// and the string "true" satisfy a boolean member — storing a wire value of a +// type every generated binding would then fail to decode. +func enumMatches(value, member any) bool { + switch got := value.(type) { + case string: + want, ok := member.(string) + return ok && got == want + case bool: + want, ok := member.(bool) + return ok && got == want + case json.Number: + return numberEqualsMember(got, member) + default: + return false + } +} + +// numberEqualsMember compares numerically, so a member written 1e6 matches a +// request sending 1000000 and an integer member 3 matches 3.0. Manifest members +// decode without UseNumber and arrive as float64; values built in tests may +// already be json.Number. +func numberEqualsMember(value json.Number, member any) bool { + var want float64 + switch typed := member.(type) { + case float64: + want = typed + case json.Number: + parsed, err := typed.Float64() + if err != nil { + return false + } + want = parsed + default: + return false + } + got, err := value.Float64() + if err != nil { + return false + } + return got == want +} + +// enumToken is a type-tagged identity for duplicate detection, so a string +// member and a numeric member that print the same are not conflated. +func enumToken(value any) string { + switch typed := value.(type) { + case string: + return "s:" + typed + case bool: + return "b:" + strconv.FormatBool(typed) + case float64: + return "n:" + strconv.FormatFloat(typed, 'g', -1, 64) + case json.Number: + if parsed, err := typed.Float64(); err == nil { + return "n:" + strconv.FormatFloat(parsed, 'g', -1, 64) + } + return "n:" + typed.String() + default: + return fmt.Sprintf("?:%v", value) + } +} + +// displayEnumValue renders a member for an error message, quoting strings so a +// reader can tell "3" from 3. +func displayEnumValue(value any) string { + switch typed := value.(type) { + case string: + return strconv.Quote(typed) + case json.Number: + return typed.String() + default: + return fmt.Sprintf("%v", value) + } +} + // strictUnmarshal rejects trailing content and, for numbers, preserves the // literal so an integer field cannot silently accept 1.5. func strictUnmarshal(raw []byte, target any) error { @@ -421,15 +578,83 @@ func strictUnmarshal(raw []byte, target any) error { if err := decoder.Decode(target); err != nil { return err } - if decoder.More() { + // Not decoder.More(): that reports whether another element follows in the + // *current* array or object, so it answers false for a stray "]" or "}" — + // exactly the bytes a caller slicing a value out of a larger document is + // most likely to hand over, which would let `true]` validate as a boolean. + // Reading the next token tolerates only end-of-input. + if _, err := decoder.Token(); !errors.Is(err, io.EOF) { return errors.New("unexpected trailing content") } return nil } -// languageTagPattern accepts well-formed BCP 47 tags of the shapes Silo -// actually stores: language, language-region, and language-script-region. -// Full RFC 5646 grammar is deliberately not implemented; anything this rejects -// is a value no client currently produces. +// languageTagPattern accepts the well-formed BCP 47 shapes real clients +// produce: language, script, region, variants, extension singletons and +// private use. The narrower language[-script][-region] form this started as +// rejected tags Android and iOS emit unprompted — `Locale.toLanguageTag()` +// appends extension subtags for a non-Gregorian calendar or non-Latin numbering +// system (`ar-EG-u-nu-latn`), and registered variants like `ca-ES-valencia` are +// ordinary user choices. var languageTagPattern = regexp.MustCompile( - `^[a-zA-Z]{2,3}(-[a-zA-Z]{4})?(-([a-zA-Z]{2}|[0-9]{3}))?$`) + `^[a-zA-Z]{2,3}(-[a-zA-Z]{4})?(-([a-zA-Z]{2}|[0-9]{3}))?` + + `(-([0-9a-zA-Z]{5,8}|[0-9][0-9a-zA-Z]{3}))*` + + `(-[0-9a-wy-zA-WY-Z](-[0-9a-zA-Z]{2,8})+)*` + + `(-[xX](-[0-9a-zA-Z]{1,8})+)?$`) + +// NormalizeLanguageTag returns the canonical BCP 47 form of a tag, or false if +// it is not well-formed. +// +// Normalization is the half that keeps the contract's promise of one stored +// value per language. Without it `en-US`, `en-us` and `EN-us` are three +// distinct rows for one preference, and audio-track matching misses on two of +// them. Underscores are accepted on input because both mobile platforms have a +// locale accessor that produces them (`Locale.identifier` on iOS, +// `Locale.toString()` on Android) and sending one is a mistake worth absorbing +// rather than a value worth rejecting. +// +// The empty string is not a language tag. "No preference" is null, which the +// nullable flag on each language setting already expresses. +func NormalizeLanguageTag(tag string) (string, bool) { + tag = strings.ReplaceAll(strings.TrimSpace(tag), "_", "-") + if !languageTagPattern.MatchString(tag) { + return "", false + } + + parts := strings.Split(tag, "-") + // Case is not significant in BCP 47, but the conventional casing is what + // every client library produces: lowercase language, Titlecase script, + // UPPERCASE region, lowercase everything else. + parts[0] = strings.ToLower(parts[0]) + inExtension := false + for i := 1; i < len(parts); i++ { + part := parts[i] + switch { + case len(part) == 1: + // A singleton opens an extension ("u", "t") or private use ("x"). + // Everything after it is extension content, so the two-letter + // region rule must stop applying — "nu" in "ar-EG-u-nu-latn" is an + // extension key, not a region. + inExtension = true + parts[i] = strings.ToLower(part) + case inExtension: + parts[i] = strings.ToLower(part) + case i == 1 && len(part) == 4 && isAlpha(part): + parts[i] = strings.ToUpper(part[:1]) + strings.ToLower(part[1:]) + case len(part) == 2 && isAlpha(part): + parts[i] = strings.ToUpper(part) + default: + parts[i] = strings.ToLower(part) + } + } + return strings.Join(parts, "-"), true +} + +func isAlpha(value string) bool { + for _, r := range value { + if (r < 'a' || r > 'z') && (r < 'A' || r > 'Z') { + return false + } + } + return true +} diff --git a/web/src/hooks/appearanceCacheOwnership.test.tsx b/web/src/hooks/appearanceCacheOwnership.test.tsx index 77ee4523..486055ee 100644 --- a/web/src/hooks/appearanceCacheOwnership.test.tsx +++ b/web/src/hooks/appearanceCacheOwnership.test.tsx @@ -1,7 +1,7 @@ -import { render } from "@testing-library/react"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { act, render } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { appearanceCache, customThemeCache, storage } from "@/utils/storage"; +import { appearanceCache, storage } from "@/utils/storage"; import { DEFAULT_THEME } from "@/lib/themes"; const mocks = vi.hoisted(() => ({ @@ -41,19 +41,33 @@ function Probe({ onRender }: { onRender: (captured: Captured) => void }) { return null; } -function renderAppearance(): Captured { - let captured: Captured | null = null; - render( +/** + * Renders the appearance providers and keeps returning the latest captured + * values, so a test can change the signed-in account and re-render the same + * tree — the account switch a running SPA actually performs. + */ +function renderAppearance() { + const latest: { current: Captured | null } = { current: null }; + // A fresh element each time: re-rendering the identical element reference + // lets React bail out, which would silently skip the account change. + const tree = () => ( { - captured = next; + latest.current = next; }} /> - , + ); - if (!captured) throw new Error("probe never rendered"); - return captured; + const { rerender } = render(tree()); + if (!latest.current) throw new Error("probe never rendered"); + return { + get captured(): Captured { + if (!latest.current) throw new Error("probe never rendered"); + return latest.current; + }, + rerender: () => rerender(tree()), + }; } /** Everything account 1 left behind on this browser. */ @@ -62,8 +76,8 @@ function seedAccountOneAppearance(): void { appearanceCache.set(KEYS.UI_TEXT_SCALE, "large", "1"); appearanceCache.set(KEYS.UI_TEXT_WEIGHT, "strong", "1"); appearanceCache.set(KEYS.UI_HIGH_CONTRAST, "true", "1"); - customThemeCache.set(KEYS.UI_CUSTOM_THEME_VARS, JSON.stringify({ "color-bg": "#ff0000" }), "1"); - customThemeCache.set(KEYS.UI_CUSTOM_CSS, "body { filter: invert(1); }", "1"); + appearanceCache.set(KEYS.UI_CUSTOM_THEME_VARS, JSON.stringify({ "color-bg": "#ff0000" }), "1"); + appearanceCache.set(KEYS.UI_CUSTOM_CSS, "body { filter: invert(1); }", "1"); } function signedInAs(id: number): void { @@ -73,7 +87,7 @@ function signedInAs(id: number): void { describe("appearance cache ownership", () => { beforeEach(() => { vi.clearAllMocks(); - Object.values(KEYS).forEach((key) => storage.remove(key)); + localStorage.clear(); mocks.useSettings.mockReturnValue({ data: {} }); mocks.useBranding.mockReturnValue({ defaultTheme: null }); }); @@ -82,7 +96,7 @@ describe("appearance cache ownership", () => { seedAccountOneAppearance(); signedInAs(2); - const captured = renderAppearance(); + const { captured } = renderAppearance(); expect(captured.theme.theme).toBe(DEFAULT_THEME); expect(captured.theme.textScale).toBe("default"); @@ -92,28 +106,26 @@ describe("appearance cache ownership", () => { expect(captured.custom.customCss).toBe(""); }); - it("drops another account's cached appearance instead of leaving it to be re-trusted", () => { + it("leaves the other account's values intact instead of deleting them", () => { seedAccountOneAppearance(); signedInAs(2); renderAppearance(); - expect(storage.get(KEYS.THEME)).toBeNull(); - expect(storage.get(KEYS.UI_TEXT_SCALE)).toBeNull(); - expect(storage.get(KEYS.UI_TEXT_WEIGHT)).toBeNull(); - expect(storage.get(KEYS.UI_HIGH_CONTRAST)).toBeNull(); - expect(storage.get(KEYS.UI_CUSTOM_THEME_VARS)).toBeNull(); - expect(storage.get(KEYS.UI_CUSTOM_CSS)).toBeNull(); - expect(appearanceCache.isTrusted("2")).toBe(true); - expect(customThemeCache.isTrusted("2")).toBe(true); + // Account 1 signing back in must still get their warm start; the previous + // design cleared these keys, which cost account 1 a default-theme flash on + // every cold start from then on. + expect(appearanceCache.get(KEYS.THEME, "1")).toBe("cobalt-studio"); + expect(appearanceCache.get(KEYS.UI_TEXT_SCALE, "1")).toBe("large"); + expect(appearanceCache.get(KEYS.UI_CUSTOM_CSS, "1")).toBe("body { filter: invert(1); }"); }); - it("still applies the admin default theme to an account that inherited a foreign cache", () => { + it("still applies the admin default theme to an account with no cached appearance", () => { seedAccountOneAppearance(); signedInAs(2); mocks.useBranding.mockReturnValue({ defaultTheme: "evergreen-studio" }); - const captured = renderAppearance(); + const { captured } = renderAppearance(); expect(captured.theme.theme).toBe("evergreen-studio"); }); @@ -122,7 +134,7 @@ describe("appearance cache ownership", () => { seedAccountOneAppearance(); signedInAs(1); - const captured = renderAppearance(); + const { captured } = renderAppearance(); expect(captured.theme.theme).toBe("cobalt-studio"); expect(captured.theme.textScale).toBe("large"); @@ -130,28 +142,26 @@ describe("appearance cache ownership", () => { expect(captured.theme.highContrast).toBe(true); expect(captured.custom.vars).toEqual({ "color-bg": "#ff0000" }); expect(captured.custom.customCss).toBe("body { filter: invert(1); }"); - expect(storage.get(KEYS.THEME)).toBe("cobalt-studio"); }); it("keeps the warm start while auth is still bootstrapping", () => { seedAccountOneAppearance(); mocks.useOptionalAuth.mockReturnValue({ loading: true, user: null }); - const captured = renderAppearance(); + const { captured } = renderAppearance(); expect(captured.theme.theme).toBe("cobalt-studio"); expect(captured.theme.textScale).toBe("large"); expect(captured.custom.customCss).toBe("body { filter: invert(1); }"); - expect(storage.get(KEYS.THEME)).toBe("cobalt-studio"); }); - it("ignores an unstamped legacy cache once an account is known", () => { + it("ignores a legacy cache written before namespacing existed", () => { storage.set(KEYS.THEME, "cobalt-studio"); storage.set(KEYS.UI_TEXT_SCALE, "large"); storage.set(KEYS.UI_CUSTOM_CSS, "body { filter: invert(1); }"); signedInAs(2); - const captured = renderAppearance(); + const { captured } = renderAppearance(); expect(captured.theme.theme).toBe(DEFAULT_THEME); expect(captured.theme.textScale).toBe("default"); @@ -169,12 +179,132 @@ describe("appearance cache ownership", () => { }, }); - const captured = renderAppearance(); + const { captured } = renderAppearance(); expect(captured.theme.theme).toBe("oxblood-noir"); expect(captured.theme.textScale).toBe("x-large"); expect(captured.custom.customCss).toBe("body { color: blue; }"); - expect(storage.get(KEYS.UI_CUSTOM_CSS)).toBe("body { color: blue; }"); - expect(customThemeCache.isTrusted("2")).toBe(true); + }); + + it("mirrors the server's appearance so the next cold start paints it", () => { + signedInAs(2); + mocks.useSettings.mockReturnValue({ + data: { + ui_theme: "oxblood-noir", + ui_text_scale: "x-large", + ui_text_weight: "strong", + ui_high_contrast: "true", + }, + }); + + renderAppearance(); + + // Without this the cache only ever held choices made on this device, so a + // user who picked their theme elsewhere flashed the default on every load. + expect(appearanceCache.get(KEYS.THEME, "2")).toBe("oxblood-noir"); + expect(appearanceCache.get(KEYS.UI_TEXT_SCALE, "2")).toBe("x-large"); + expect(appearanceCache.get(KEYS.UI_TEXT_WEIGHT, "2")).toBe("strong"); + expect(appearanceCache.get(KEYS.UI_HIGH_CONTRAST, "2")).toBe("true"); + }); + + it("does not mirror a theme the user never chose, so the admin default still moves", () => { + signedInAs(2); + mocks.useBranding.mockReturnValue({ defaultTheme: "evergreen-studio" }); + + renderAppearance(); + + expect(appearanceCache.get(KEYS.THEME, "2")).toBeNull(); + }); + + it("stops painting the previous account when the signed-in account changes", () => { + seedAccountOneAppearance(); + signedInAs(1); + + const view = renderAppearance(); + expect(view.captured.theme.theme).toBe("cobalt-studio"); + + act(() => { + signedInAs(2); + view.rerender(); + }); + + expect(view.captured.theme.theme).toBe(DEFAULT_THEME); + expect(view.captured.theme.textScale).toBe("default"); + expect(view.captured.theme.highContrast).toBe(false); + expect(view.captured.custom.vars).toEqual({}); + expect(view.captured.custom.customCss).toBe(""); + }); + + it("restores the first account's look when they sign back in", () => { + seedAccountOneAppearance(); + signedInAs(2); + + const view = renderAppearance(); + expect(view.captured.theme.theme).toBe(DEFAULT_THEME); + + act(() => { + signedInAs(1); + view.rerender(); + }); + + expect(view.captured.theme.theme).toBe("cobalt-studio"); + expect(view.captured.theme.textScale).toBe("large"); + expect(view.captured.custom.customCss).toBe("body { filter: invert(1); }"); + }); +}); + +describe("custom theme debounced writes", () => { + beforeEach(() => { + vi.clearAllMocks(); + localStorage.clear(); + mocks.useSettings.mockReturnValue({ data: {} }); + mocks.useBranding.mockReturnValue({ defaultTheme: null }); + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("drops a pending write when the account changes mid-debounce", () => { + signedInAs(1); + const view = renderAppearance(); + + act(() => { + view.captured.custom.setCustomCss("body { filter: invert(1); }"); + }); + + // Account 1 signs out and account 2 signs in inside the 1s debounce. + act(() => { + signedInAs(2); + view.rerender(); + }); + act(() => { + vi.advanceTimersByTime(2000); + }); + + // The timer captured account 1's owner and account 2's live session. It + // must not store account 1's CSS against account 2. + expect(mocks.mutate).not.toHaveBeenCalled(); + expect(appearanceCache.get(KEYS.UI_CUSTOM_CSS, "2")).toBeNull(); + expect(view.captured.custom.customCss).toBe(""); + }); + + it("still persists a write that is not interrupted", () => { + signedInAs(1); + const view = renderAppearance(); + + act(() => { + view.captured.custom.setCustomCss("body { color: red; }"); + }); + act(() => { + vi.advanceTimersByTime(2000); + }); + + expect(mocks.mutate).toHaveBeenCalledWith({ + key: "ui_custom_css", + value: "body { color: red; }", + }); + expect(appearanceCache.get(KEYS.UI_CUSTOM_CSS, "1")).toBe("body { color: red; }"); }); }); diff --git a/web/src/hooks/themePreferences.ts b/web/src/hooks/themePreferences.ts index cee0532e..2f0cc72e 100644 --- a/web/src/hooks/themePreferences.ts +++ b/web/src/hooks/themePreferences.ts @@ -11,13 +11,16 @@ export interface AppearanceAuth { } /** - * The account that owns the device-local appearance caches, or null while auth - * is bootstrapping or nobody is signed in. + * The namespace that owns the device-local appearance caches, or null while + * auth is bootstrapping or nobody is signed in. * * Appearance settings are user-scoped server side (`GET /settings` resolves * against `user_settings` for the authenticated user), so the user id is the - * right owner token today. Widen this one function if appearance ever moves to - * profile scope — profiles on one account share a user id. + * right owner token today. When appearance moves to profile scope — the + * settings contract puts `ui.theme` at `profile`, and profiles on one account + * share a user id — appending the active profile id here is the whole change: + * every cache read and write in the app resolves its namespace through this + * function, so no call site can be left behind. */ export function appearanceCacheOwner({ loading, user }: AppearanceAuth): string | null { return !loading && user ? String(user.id) : null; diff --git a/web/src/hooks/useCustomTheme.ts b/web/src/hooks/useCustomTheme.ts index 2d6be3c5..93f28617 100644 --- a/web/src/hooks/useCustomTheme.ts +++ b/web/src/hooks/useCustomTheme.ts @@ -2,7 +2,7 @@ import { useCallback, useEffect, useRef, useState } from "react"; import { useSettings, useSetSetting } from "@/hooks/queries/settings"; import { useOptionalAuth } from "@/hooks/useAuth"; import { appearanceCacheOwner } from "@/hooks/themePreferences"; -import { customThemeCache, storage } from "@/utils/storage"; +import { appearanceCache, storage } from "@/utils/storage"; import { parseVarsJson } from "@/lib/themeExport"; import { sanitizeCss } from "@/lib/cssSanitizer"; import type { ThemeToken } from "@/lib/themeTokens"; @@ -28,8 +28,6 @@ interface UseCustomThemeResult { isDirty: boolean; } -const EMPTY_VARS: ThemeVarOverrides = {}; - export function useCustomTheme(): UseCustomThemeResult { const auth = useOptionalAuth(); // Owner of the localStorage warm start; null while auth bootstraps or when @@ -39,9 +37,6 @@ export function useCustomTheme(): UseCustomThemeResult { user: auth?.user ? { id: auth.user.id } : null, }); const loadApi = cacheOwner !== null; - // Read once per render, before the effects below re-stamp the cache, so every - // useCustomTheme instance in this render pass agrees on the answer. - const cacheTrusted = customThemeCache.isTrusted(cacheOwner); // API values const { data: apiSettings } = useSettings({ enabled: loadApi }); @@ -51,10 +46,10 @@ export function useCustomTheme(): UseCustomThemeResult { // Local draft state (for instant updates without waiting for API) const [localVars, setLocalVars] = useState(() => - parseVarsJson(customThemeCache.get(storage.KEYS.UI_CUSTOM_THEME_VARS, cacheOwner)), + parseVarsJson(appearanceCache.get(storage.KEYS.UI_CUSTOM_THEME_VARS, cacheOwner)), ); const [localCss, setLocalCss] = useState( - () => customThemeCache.get(storage.KEYS.UI_CUSTOM_CSS, cacheOwner) ?? "", + () => appearanceCache.get(storage.KEYS.UI_CUSTOM_CSS, cacheOwner) ?? "", ); const [isDirty, setIsDirty] = useState(false); @@ -62,37 +57,50 @@ export function useCustomTheme(): UseCustomThemeResult { const varsTimerRef = useRef | undefined>(undefined); const cssTimerRef = useRef | undefined>(undefined); - // Another account's custom theme is never a valid starting point: drop it (and - // take ownership of the now-empty cache) as soon as we know who is signed in. - // Declared before the API sync effects so it can never wipe values they just - // wrote for the new owner. + // A debounced persist closes over the account it was scheduled for. Drop any + // pending write when the account changes or the editor unmounts: otherwise a + // timer armed by the previous account fires against the new account's session + // and stores their predecessor's CSS under their name. useEffect(() => { - if (cacheOwner === null || cacheTrusted) return; - customThemeCache.clear(cacheOwner); - setLocalVars(EMPTY_VARS); - setLocalCss(""); - }, [cacheOwner, cacheTrusted]); + return () => { + clearTimeout(varsTimerRef.current); + clearTimeout(cssTimerRef.current); + }; + }, [cacheOwner]); + + // This state was seeded for whoever was signed in when the hook mounted. + // Re-seed from the new owner's namespace when the account changes, so an + // account switch without a reload stops rendering the previous account's + // tokens even before their own settings arrive. Adjusted during render, so + // there is no frame in which the new account sees the old one's theme. + const [seededOwner, setSeededOwner] = useState(cacheOwner); + if (seededOwner !== cacheOwner) { + setSeededOwner(cacheOwner); + setLocalVars(parseVarsJson(appearanceCache.get(storage.KEYS.UI_CUSTOM_THEME_VARS, cacheOwner))); + setLocalCss(appearanceCache.get(storage.KEYS.UI_CUSTOM_CSS, cacheOwner) ?? ""); + setIsDirty(false); + } // Sync API values into local state when they arrive useEffect(() => { if (loadApi && apiVars !== undefined) { const parsed = parseVarsJson(apiVars); setLocalVars(parsed); - customThemeCache.set(storage.KEYS.UI_CUSTOM_THEME_VARS, JSON.stringify(parsed), cacheOwner); + appearanceCache.set(storage.KEYS.UI_CUSTOM_THEME_VARS, JSON.stringify(parsed), cacheOwner); } }, [loadApi, apiVars, cacheOwner]); useEffect(() => { if (loadApi && apiCss !== undefined && apiCss !== null) { setLocalCss(apiCss); - customThemeCache.set(storage.KEYS.UI_CUSTOM_CSS, apiCss, cacheOwner); + appearanceCache.set(storage.KEYS.UI_CUSTOM_CSS, apiCss, cacheOwner); } }, [loadApi, apiCss, cacheOwner]); const persistVars = useCallback( (vars: ThemeVarOverrides) => { const json = JSON.stringify(vars); - customThemeCache.set(storage.KEYS.UI_CUSTOM_THEME_VARS, json, cacheOwner); + appearanceCache.set(storage.KEYS.UI_CUSTOM_THEME_VARS, json, cacheOwner); settingMutation.mutate({ key: "ui_custom_theme_vars", value: json }); }, [settingMutation, cacheOwner], @@ -101,7 +109,7 @@ export function useCustomTheme(): UseCustomThemeResult { const persistCss = useCallback( (css: string) => { const safe = sanitizeCss(css); - customThemeCache.set(storage.KEYS.UI_CUSTOM_CSS, safe, cacheOwner); + appearanceCache.set(storage.KEYS.UI_CUSTOM_CSS, safe, cacheOwner); settingMutation.mutate({ key: "ui_custom_css", value: safe }); }, [settingMutation, cacheOwner], @@ -174,10 +182,8 @@ export function useCustomTheme(): UseCustomThemeResult { ); return { - // Until the cache is proven to be ours, render nothing custom rather than - // the previous account's tokens and CSS. - vars: cacheTrusted ? localVars : EMPTY_VARS, - customCss: cacheTrusted ? localCss : "", + vars: localVars, + customCss: localCss, setVar, resetVar, setAllVars, diff --git a/web/src/hooks/useDateTimeFormat.tsx b/web/src/hooks/useDateTimeFormat.tsx index ad6ed44a..61a40f8b 100644 --- a/web/src/hooks/useDateTimeFormat.tsx +++ b/web/src/hooks/useDateTimeFormat.tsx @@ -15,7 +15,8 @@ import type { } from "@/lib/datetime"; import { useSettings, useSetSetting } from "@/hooks/queries/settings"; import { useOptionalAuth } from "@/hooks/useAuth"; -import { dateTimeFormatCache, storage } from "@/utils/storage"; +import { appearanceCacheOwner } from "@/hooks/themePreferences"; +import { appearanceCache, storage } from "@/utils/storage"; export const DATE_FORMAT_SETTING_KEY = "ui.date_format"; export const TIME_FORMAT_SETTING_KEY = "ui.time_format"; @@ -23,9 +24,14 @@ export const TIME_FORMAT_SETTING_KEY = "ui.time_format"; // Seed the shared formatter state from localStorage at module load, before the // first render, so an app booting straight into a date-heavy page doesn't // paint in the wrong format while the settings request is in flight. +// +// Auth has not resolved this early, so this reads through the same namespace +// resolution as every other access — the account that last wrote here — rather +// than the bare key. Reading the bare key would seed the formatter with +// whichever account happened to write before namespacing existed. setDateTimeFormatPreferences({ - dateFormat: parseDateFormatPreference(storage.get(storage.KEYS.UI_DATE_FORMAT)), - timeFormat: parseTimeFormatPreference(storage.get(storage.KEYS.UI_TIME_FORMAT)), + dateFormat: parseDateFormatPreference(appearanceCache.get(storage.KEYS.UI_DATE_FORMAT, null)), + timeFormat: parseTimeFormatPreference(appearanceCache.get(storage.KEYS.UI_TIME_FORMAT, null)), }); interface DateTimeFormatContextValue { @@ -44,8 +50,13 @@ const DateTimeFormatContext = createContext(n */ export function DateTimeFormatProvider({ children }: { children: ReactNode }) { const auth = useOptionalAuth(); - const authUserId = auth && !auth.loading && auth.user ? String(auth.user.id) : null; - const loadApiSettings = authUserId !== null; + // Same owner token as the theme caches, from the same function, so widening + // ownership can never move one provider and leave this one behind. + const cacheOwner = appearanceCacheOwner({ + loading: auth?.loading ?? false, + user: auth?.user ? { id: auth.user.id } : null, + }); + const loadApiSettings = cacheOwner !== null; const { data: apiSettings } = useSettings({ enabled: loadApiSettings }); const settingMutation = useSetSetting(); @@ -54,48 +65,48 @@ export function DateTimeFormatProvider({ children }: { children: ReactNode }) { // missing key means the user has no preference (auto), not "fall back to // whatever this device saw last" — and rollback of a failed save flows back // through the query cache. Until then (including when the settings request - // fails), the localStorage warm start is only trusted if it was mirrored for - // this same user; another account's device-local preference must not leak in. + // fails), fall back to this account's own mirrored values rather than the + // module-level seed, which ran before auth resolved and on a shared browser + // may hold the account that used this device last. const apiLoaded = loadApiSettings && apiSettings !== undefined; - const localTrusted = dateTimeFormatCache.isTrusted(authUserId); const dateFormat = apiLoaded ? parseDateFormatPreference(apiSettings[DATE_FORMAT_SETTING_KEY]) - : localTrusted - ? local.dateFormat - : "auto"; + : loadApiSettings + ? parseDateFormatPreference(appearanceCache.get(storage.KEYS.UI_DATE_FORMAT, cacheOwner)) + : local.dateFormat; const timeFormat = apiLoaded ? parseTimeFormatPreference(apiSettings[TIME_FORMAT_SETTING_KEY]) - : localTrusted - ? local.timeFormat - : "auto"; + : loadApiSettings + ? parseTimeFormatPreference(appearanceCache.get(storage.KEYS.UI_TIME_FORMAT, cacheOwner)) + : local.timeFormat; useEffect(() => { setDateTimeFormatPreferences({ dateFormat, timeFormat }); - // Mirror the resolved values (tagged with their owner) so the next load on - // this device paints in the right format before the settings request + // Mirror the resolved values into this account's namespace so the next load + // on this device paints in the right format before the settings request // resolves. if (apiLoaded) { - dateTimeFormatCache.set(storage.KEYS.UI_DATE_FORMAT, dateFormat, authUserId); - dateTimeFormatCache.set(storage.KEYS.UI_TIME_FORMAT, timeFormat, authUserId); + appearanceCache.set(storage.KEYS.UI_DATE_FORMAT, dateFormat, cacheOwner); + appearanceCache.set(storage.KEYS.UI_TIME_FORMAT, timeFormat, cacheOwner); } - }, [dateFormat, timeFormat, apiLoaded, authUserId]); + }, [dateFormat, timeFormat, apiLoaded, cacheOwner]); const setDateFormat = useCallback( (value: DateFormatPreference) => { setDateTimeFormatPreferences({ ...getDateTimeFormatPreferences(), dateFormat: value }); - dateTimeFormatCache.set(storage.KEYS.UI_DATE_FORMAT, value, authUserId); + appearanceCache.set(storage.KEYS.UI_DATE_FORMAT, value, cacheOwner); settingMutation.mutate({ key: DATE_FORMAT_SETTING_KEY, value }); }, - [settingMutation, authUserId], + [settingMutation, cacheOwner], ); const setTimeFormat = useCallback( (value: TimeFormatPreference) => { setDateTimeFormatPreferences({ ...getDateTimeFormatPreferences(), timeFormat: value }); - dateTimeFormatCache.set(storage.KEYS.UI_TIME_FORMAT, value, authUserId); + appearanceCache.set(storage.KEYS.UI_TIME_FORMAT, value, cacheOwner); settingMutation.mutate({ key: TIME_FORMAT_SETTING_KEY, value }); }, - [settingMutation, authUserId], + [settingMutation, cacheOwner], ); return ( diff --git a/web/src/hooks/useTheme.tsx b/web/src/hooks/useTheme.tsx index 205876c4..369a04cc 100644 --- a/web/src/hooks/useTheme.tsx +++ b/web/src/hooks/useTheme.tsx @@ -1,7 +1,6 @@ import { createContext, useContext, useEffect, useState, useCallback } from "react"; import type { ReactNode } from "react"; import type { ThemeId } from "@/lib/themes"; -import { DEFAULT_THEME } from "@/lib/themes"; import { useSettings, useSetSetting } from "@/hooks/queries/settings"; import { useOptionalAuth } from "@/hooks/useAuth"; import { useBranding } from "@/hooks/useBranding"; @@ -57,9 +56,6 @@ export function ThemeProvider({ children }: { children: ReactNode }) { user: auth?.user ? { id: auth.user.id } : null, }); const loadApiTheme = cacheOwner !== null; - // Read once per render, before any effect below re-stamps the cache, so every - // ThemeProvider in this render pass agrees on whether the cache is ours. - const cacheTrusted = appearanceCache.isTrusted(cacheOwner); const [themePreference, setThemePreference] = useState(() => getInitialTheme(cacheOwner), @@ -75,16 +71,29 @@ export function ThemeProvider({ children }: { children: ReactNode }) { parseHighContrast(appearanceCache.get(storage.KEYS.UI_HIGH_CONTRAST, cacheOwner)), ); - // Another account's appearance is never a valid starting point: drop it (and - // take ownership of the now-empty cache) as soon as we know who is signed in. - useEffect(() => { - if (cacheOwner === null || cacheTrusted) return; - appearanceCache.clear(cacheOwner); - setThemePreference(DEFAULT_THEME); - setTextScalePreference("default"); - setTextWeightPreference("default"); - setHighContrastPreference(false); - }, [cacheOwner, cacheTrusted]); + // This state was seeded for whoever was signed in when the provider mounted. + // Re-seed from the new owner's namespace when the account changes, so signing + // out and back in as someone else without a reload stops painting the + // previous account's look. Values are namespaced, so this reads the new + // account's own warm start rather than falling back to defaults. + // + // Adjusted during render rather than in an effect: React re-runs this pass + // before committing, so the new account never gets a frame painted with the + // previous one's appearance. + const [seededOwner, setSeededOwner] = useState(cacheOwner); + if (seededOwner !== cacheOwner) { + setSeededOwner(cacheOwner); + setThemePreference(getInitialTheme(cacheOwner)); + setTextScalePreference( + parseTextScale(appearanceCache.get(storage.KEYS.UI_TEXT_SCALE, cacheOwner)), + ); + setTextWeightPreference( + parseTextWeight(appearanceCache.get(storage.KEYS.UI_TEXT_WEIGHT, cacheOwner)), + ); + setHighContrastPreference( + parseHighContrast(appearanceCache.get(storage.KEYS.UI_HIGH_CONTRAST, cacheOwner)), + ); + } // Load persisted setting from API (user-scoped) const { data: apiSettings } = useSettings({ enabled: loadApiTheme }); @@ -98,13 +107,10 @@ export function ThemeProvider({ children }: { children: ReactNode }) { // preference of their own (no stored local choice and no profile ui_theme). // A user's explicit choice always wins, preserving the per-user layering. const { defaultTheme: adminDefaultTheme } = useBranding(); - // Values cached by another account must not stand in for the signed-in - // account's missing preferences — that would both show them someone else's - // appearance and suppress the admin default theme they should be getting. - const localTheme = cacheTrusted ? themePreference : DEFAULT_THEME; - const localTextScale = cacheTrusted ? textScalePreference : "default"; - const localTextWeight = cacheTrusted ? textWeightPreference : "default"; - const localHighContrast = cacheTrusted ? highContrastPreference : false; + const localTheme = themePreference; + const localTextScale = textScalePreference; + const localTextWeight = textWeightPreference; + const localHighContrast = highContrastPreference; const hasStoredThemeChoice = appearanceCache.get(storage.KEYS.THEME, cacheOwner) != null; const fallbackTheme: ThemeId = !hasStoredThemeChoice && isValidTheme(adminDefaultTheme) ? adminDefaultTheme : localTheme; @@ -121,6 +127,37 @@ export function ThemeProvider({ children }: { children: ReactNode }) { ? parseHighContrast(apiHighContrast ?? String(localHighContrast)) : localHighContrast; + // Mirror the server's values into this account's namespace so the next cold + // start paints them before the settings request resolves. Without this the + // cache would only ever hold choices made on this device, and a user who + // picked their theme elsewhere would flash the default on every load. + // + // Only keys the user actually has a stored preference for are mirrored: the + // absence of a cached theme is what lets the admin default apply, so writing + // a resolved-but-unchosen value here would silently pin them to whatever the + // default happened to be the first time they loaded the app. + useEffect(() => { + if (!loadApiTheme || apiSettings === undefined) return; + if (isValidTheme(apiTheme)) appearanceCache.set(storage.KEYS.THEME, apiTheme, cacheOwner); + if (apiTextScale != null) { + appearanceCache.set(storage.KEYS.UI_TEXT_SCALE, apiTextScale, cacheOwner); + } + if (apiTextWeight != null) { + appearanceCache.set(storage.KEYS.UI_TEXT_WEIGHT, apiTextWeight, cacheOwner); + } + if (apiHighContrast != null) { + appearanceCache.set(storage.KEYS.UI_HIGH_CONTRAST, apiHighContrast, cacheOwner); + } + }, [ + loadApiTheme, + apiSettings, + apiTheme, + apiTextScale, + apiTextWeight, + apiHighContrast, + cacheOwner, + ]); + useEffect(() => { applyThemeToDOM(previewThemeState ?? theme); }, [previewThemeState, theme]); diff --git a/web/src/utils/storage.test.ts b/web/src/utils/storage.test.ts index c459f668..1a1ae148 100644 --- a/web/src/utils/storage.test.ts +++ b/web/src/utils/storage.test.ts @@ -1,26 +1,16 @@ import { beforeEach, describe, expect, it } from "vitest"; -import { appearanceCache, customThemeCache, dateTimeFormatCache, storage } from "./storage"; +import { appearanceCache, storage } from "./storage"; const KEYS = storage.KEYS; -describe("owned caches", () => { +describe("appearance cache namespacing", () => { beforeEach(() => { - Object.values(KEYS).forEach((key) => storage.remove(key)); + localStorage.clear(); }); - it("trusts the cache while nobody is known to be signed in", () => { - storage.set(KEYS.THEME, "cobalt-studio"); - storage.set(KEYS.UI_APPEARANCE_OWNER, "1"); - - expect(appearanceCache.isTrusted(null)).toBe(true); - expect(appearanceCache.get(KEYS.THEME, null)).toBe("cobalt-studio"); - }); - - it("trusts the cache for the account that stamped it", () => { + it("reads back what the same account wrote", () => { appearanceCache.set(KEYS.THEME, "cobalt-studio", "1"); - expect(storage.get(KEYS.UI_APPEARANCE_OWNER)).toBe("1"); - expect(appearanceCache.isTrusted("1")).toBe(true); expect(appearanceCache.get(KEYS.THEME, "1")).toBe("cobalt-studio"); }); @@ -28,61 +18,68 @@ describe("owned caches", () => { appearanceCache.set(KEYS.THEME, "cobalt-studio", "1"); appearanceCache.set(KEYS.UI_TEXT_SCALE, "large", "1"); - expect(appearanceCache.isTrusted("2")).toBe(false); expect(appearanceCache.get(KEYS.THEME, "2")).toBeNull(); expect(appearanceCache.get(KEYS.UI_TEXT_SCALE, "2")).toBeNull(); }); - it("does not trust an unstamped legacy cache for a known account", () => { + it("keeps both accounts' values, so returning to the first still warm starts", () => { + appearanceCache.set(KEYS.THEME, "cobalt-studio", "1"); + appearanceCache.set(KEYS.THEME, "oxblood-noir", "2"); + + expect(appearanceCache.get(KEYS.THEME, "1")).toBe("cobalt-studio"); + expect(appearanceCache.get(KEYS.THEME, "2")).toBe("oxblood-noir"); + }); + + it("ignores values written before namespacing existed", () => { storage.set(KEYS.THEME, "cobalt-studio"); - expect(appearanceCache.isTrusted("1")).toBe(false); expect(appearanceCache.get(KEYS.THEME, "1")).toBeNull(); }); - it("clear() drops every member value and hands the empty cache to the new owner", () => { - appearanceCache.set(KEYS.THEME, "cobalt-studio", "1"); - appearanceCache.set(KEYS.UI_TEXT_SCALE, "large", "1"); - appearanceCache.set(KEYS.UI_TEXT_WEIGHT, "strong", "1"); - appearanceCache.set(KEYS.UI_HIGH_CONTRAST, "true", "1"); - - appearanceCache.clear("2"); - - expect(storage.get(KEYS.THEME)).toBeNull(); - expect(storage.get(KEYS.UI_TEXT_SCALE)).toBeNull(); - expect(storage.get(KEYS.UI_TEXT_WEIGHT)).toBeNull(); - expect(storage.get(KEYS.UI_HIGH_CONTRAST)).toBeNull(); - expect(appearanceCache.isTrusted("2")).toBe(true); - expect(appearanceCache.isTrusted("1")).toBe(false); - }); - - it("clear() without an owner leaves the cache unstamped", () => { + it("falls back to the last account that wrote while nobody is signed in", () => { appearanceCache.set(KEYS.THEME, "cobalt-studio", "1"); - appearanceCache.clear(null); - - expect(storage.get(KEYS.THEME)).toBeNull(); - expect(storage.get(KEYS.UI_APPEARANCE_OWNER)).toBeNull(); - expect(appearanceCache.isTrusted("1")).toBe(false); + expect(appearanceCache.get(KEYS.THEME, null)).toBe("cobalt-studio"); }); - it("keeps each cache group's ownership independent", () => { + it("follows the pointer to the most recent account, not the first", () => { appearanceCache.set(KEYS.THEME, "cobalt-studio", "1"); - customThemeCache.set(KEYS.UI_CUSTOM_CSS, "body{}", "2"); - dateTimeFormatCache.set(KEYS.UI_DATE_FORMAT, "iso", "3"); + appearanceCache.set(KEYS.THEME, "oxblood-noir", "2"); - expect(appearanceCache.isTrusted("1")).toBe(true); - expect(appearanceCache.isTrusted("2")).toBe(false); - expect(customThemeCache.isTrusted("2")).toBe(true); - expect(customThemeCache.isTrusted("1")).toBe(false); - expect(dateTimeFormatCache.isTrusted("3")).toBe(true); - expect(dateTimeFormatCache.isTrusted("1")).toBe(false); + expect(appearanceCache.get(KEYS.THEME, null)).toBe("oxblood-noir"); }); - it("stamps nothing when there is no owner to stamp", () => { + it("keeps a never-signed-in device's values in their own namespace", () => { appearanceCache.set(KEYS.THEME, "cobalt-studio", null); - expect(storage.get(KEYS.THEME)).toBe("cobalt-studio"); - expect(storage.get(KEYS.UI_APPEARANCE_OWNER)).toBeNull(); + expect(appearanceCache.get(KEYS.THEME, null)).toBe("cobalt-studio"); + // Not the bare key, and not visible to a real account. + expect(storage.get(KEYS.THEME)).toBeNull(); + expect(appearanceCache.get(KEYS.THEME, "1")).toBeNull(); + }); + + it("routes a signed-out write into the last account's namespace without moving the pointer", () => { + appearanceCache.set(KEYS.THEME, "cobalt-studio", "1"); + appearanceCache.set(KEYS.THEME, "evergreen-studio", null); + + // Reads and writes resolve their namespace the same way, so a theme change + // made on the login screen is the one the next read sees. It lands in the + // last account's cache, which is only a warm start: their server value + // still wins once the settings request resolves. + expect(storage.get(KEYS.UI_CACHE_OWNER)).toBe("1"); + expect(appearanceCache.get(KEYS.THEME, null)).toBe("evergreen-studio"); + expect(appearanceCache.get(KEYS.THEME, "1")).toBe("evergreen-studio"); + expect(appearanceCache.get(KEYS.THEME, "2")).toBeNull(); + }); + + it("keeps every key in the group independently namespaced", () => { + appearanceCache.set(KEYS.THEME, "cobalt-studio", "1"); + appearanceCache.set(KEYS.UI_CUSTOM_CSS, "body{}", "1"); + appearanceCache.set(KEYS.UI_DATE_FORMAT, "iso", "2"); + + expect(appearanceCache.get(KEYS.UI_CUSTOM_CSS, "1")).toBe("body{}"); + expect(appearanceCache.get(KEYS.UI_CUSTOM_CSS, "2")).toBeNull(); + expect(appearanceCache.get(KEYS.UI_DATE_FORMAT, "2")).toBe("iso"); + expect(appearanceCache.get(KEYS.UI_DATE_FORMAT, "1")).toBeNull(); }); }); diff --git a/web/src/utils/storage.ts b/web/src/utils/storage.ts index 7fbd529a..e4086e6a 100644 --- a/web/src/utils/storage.ts +++ b/web/src/utils/storage.ts @@ -18,16 +18,14 @@ const STORAGE_KEYS = { UI_CUSTOM_THEME_VARS: "silo-custom-theme-vars", UI_DATE_FORMAT: "silo-ui-date-format", UI_TIME_FORMAT: "silo-ui-time-format", - UI_DATETIME_FORMAT_OWNER: "silo-ui-datetime-format-owner", - UI_APPEARANCE_OWNER: "silo-ui-appearance-owner", - UI_CUSTOM_THEME_OWNER: "silo-ui-custom-theme-owner", + UI_CACHE_OWNER: "silo-ui-cache-owner", UI_CUSTOM_CSS: "silo-custom-css", CALENDAR_PRESET: "calendar:preset", } as const; export type StorageKey = (typeof STORAGE_KEYS)[keyof typeof STORAGE_KEYS]; -function get(key: StorageKey): string | null { +function getRaw(key: string): string | null { try { return localStorage.getItem(key); } catch { @@ -35,7 +33,7 @@ function get(key: StorageKey): string | null { } } -function set(key: StorageKey, value: string): void { +function setRaw(key: string, value: string): void { try { localStorage.setItem(key, value); } catch { @@ -43,6 +41,14 @@ function set(key: StorageKey, value: string): void { } } +function get(key: StorageKey): string | null { + return getRaw(key); +} + +function set(key: StorageKey, value: string): void { + setRaw(key, value); +} + function remove(key: StorageKey): void { try { localStorage.removeItem(key); @@ -54,80 +60,57 @@ function remove(key: StorageKey): void { export const storage = { KEYS: STORAGE_KEYS, get, set, remove }; /** - * A group of localStorage keys that mirror server-side, per-account settings so - * the UI can paint before the settings request resolves. - * - * Every write stamps the account that owns the values, and reads are only - * honored for that same account: browsers are shared, and a second account - * signing in must never inherit the first account's cached values. + * Namespace used before anyone has ever signed in on this browser. Values + * written here are the device's own defaults, not any account's. */ -export interface OwnedCache { - /** - * Whether the cached values may be applied to `owner`. - * - * A `null` owner (auth still bootstrapping, or signed out) trusts the cache so - * the warm start still paints. An unstamped cache — written before ownership - * tagging existed — is never trusted for a known account; those users take a - * one-time reset instead of a chance of seeing someone else's settings. - */ - isTrusted: (owner: string | null) => boolean; - /** The cached value, or null when the cache belongs to a different account. */ - get: (key: StorageKey, owner: string | null) => string | null; - /** Write a value and stamp `owner` as the cache's owner. */ - set: (key: StorageKey, value: string, owner: string | null) => void; - /** Drop every cached value and hand the empty cache to `owner`. */ - clear: (owner: string | null) => void; +const DEVICE_NAMESPACE = "device"; + +/** + * Which namespace a read or write belongs to. + * + * A known account always uses its own. A `null` owner means auth is still + * bootstrapping or nobody is signed in, and we fall back to the last account + * that wrote here so the login screen and the pre-auth first paint keep the + * look this device last used. That fallback cannot leak into a signed-in + * session: the moment auth resolves, the owner is exact. + */ +function namespaceFor(owner: string | null): string { + if (owner !== null) return owner; + return getRaw(STORAGE_KEYS.UI_CACHE_OWNER) ?? DEVICE_NAMESPACE; } -function createOwnedCache(ownerKey: StorageKey, memberKeys: readonly StorageKey[]): OwnedCache { - function stamp(owner: string | null): void { - if (owner === null) return; - set(ownerKey, owner); - } - - function isTrusted(owner: string | null): boolean { - return owner === null || get(ownerKey) === owner; - } - - return { - isTrusted, - get: (key, owner) => (isTrusted(owner) ? get(key) : null), - set: (key, value, owner) => { - set(key, value); - stamp(owner); - }, - clear: (owner) => { - memberKeys.forEach((key) => remove(key)); - if (owner === null) { - remove(ownerKey); - } else { - stamp(owner); - } - }, - }; -} - -// Each group carries its own owner stamp rather than sharing one. The groups are -// written by different hooks whose effects run in a fixed nesting order, so a -// shared stamp would let whichever hook resolved first vouch for another hook's -// still-stale values. - -/** Theme, text scale, text weight and high contrast (written by useTheme). */ -export const appearanceCache = createOwnedCache(STORAGE_KEYS.UI_APPEARANCE_OWNER, [ - STORAGE_KEYS.THEME, - STORAGE_KEYS.UI_TEXT_SCALE, - STORAGE_KEYS.UI_TEXT_WEIGHT, - STORAGE_KEYS.UI_HIGH_CONTRAST, -]); - -/** Custom theme token overrides and raw CSS (written by useCustomTheme). */ -export const customThemeCache = createOwnedCache(STORAGE_KEYS.UI_CUSTOM_THEME_OWNER, [ - STORAGE_KEYS.UI_CUSTOM_THEME_VARS, - STORAGE_KEYS.UI_CUSTOM_CSS, -]); - -/** Date and time format preferences (written by DateTimeFormatProvider). */ -export const dateTimeFormatCache = createOwnedCache(STORAGE_KEYS.UI_DATETIME_FORMAT_OWNER, [ - STORAGE_KEYS.UI_DATE_FORMAT, - STORAGE_KEYS.UI_TIME_FORMAT, -]); +/** + * Device-local mirrors of server-side, per-account settings, so the UI can + * paint before the settings request resolves. Covers theme, text scale, text + * weight, high contrast, custom theme tokens, custom CSS, and date/time format. + * + * Values are namespaced by the account that owns them (`silo-theme:7`), so a + * second account signing in on a shared browser simply finds nothing where the + * first account's values would have been. A miss is just a miss: every caller + * already parses a missing value into the correct default, and the settings + * response repopulates the namespace when it lands. + * + * Namespacing rather than tagging-and-clearing matters for three reasons. + * Nothing is ever deleted, so returning to the first account still paints their + * look with no cold start. There is no shared stamp for a second tab, a stale + * debounce timer, or an out-of-order effect to race on. And widening ownership + * — appearance is user-scoped server side today, but the settings contract + * moves it to profile scope — is a change to `appearanceCacheOwner` alone, + * which no caller can forget to apply. + * + * Values written before namespacing existed sit at the bare key and are simply + * ignored. Those users take one cold paint, after which the mirror below has + * repopulated their namespace from the server, which holds all of these + * settings anyway. + */ +export const appearanceCache = { + /** The cached value for `owner`, or null when they have none. */ + get(key: StorageKey, owner: string | null): string | null { + return getRaw(`${key}:${namespaceFor(owner)}`); + }, + /** Write a value into `owner`'s namespace. */ + set(key: StorageKey, value: string, owner: string | null): void { + setRaw(`${key}:${namespaceFor(owner)}`, value); + if (owner !== null) setRaw(STORAGE_KEYS.UI_CACHE_OWNER, owner); + }, +};