diff --git a/cmd/silo/main.go b/cmd/silo/main.go index bb5f4a7a..b3ccf086 100644 --- a/cmd/silo/main.go +++ b/cmd/silo/main.go @@ -401,7 +401,10 @@ func main() { // 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() + contract, err := settingscontract.Load() + if err != nil { + log.Fatalf("settings contract: %v", err) + } contractETag, err := settingscontract.ETag() if err != nil { log.Fatalf("settings contract: %v", err) diff --git a/internal/api/handlers/settings_contract_test.go b/internal/api/handlers/settings_contract_test.go index 341e08be..bdf8b2a4 100644 --- a/internal/api/handlers/settings_contract_test.go +++ b/internal/api/handlers/settings_contract_test.go @@ -78,17 +78,28 @@ func TestRegistryDefaultsMatchTheContract(t *testing.T) { } 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 null case is settled before scalarDefault, which rejects null + // as non-scalar. Asking it first skipped the subtest and left the + // comparison below unreachable, so a nullable contract default could + // drift from the registry without failing anything. + // // 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" { + // nullable. Those are the same statement, not a disagreement — but + // null against a non-empty registry default is a real one. + if strings.TrimSpace(string(def.DefaultValue)) == "null" { + if spec.DefaultValue != "" { + t.Errorf("default disagrees: settingsRegistry has %q, contract has null", + spec.DefaultValue) + } return } + + contractDefault, err := scalarDefault(def.DefaultValue) + if err != nil { + t.Skipf("contract default is not a scalar: %s", def.DefaultValue) + } if spec.DefaultValue != contractDefault { t.Errorf("default disagrees: settingsRegistry has %q, contract has %q", spec.DefaultValue, contractDefault) diff --git a/internal/audiobooks/abs_smart_collection_store.go b/internal/audiobooks/abs_smart_collection_store.go index bf3e65b3..498207ae 100644 --- a/internal/audiobooks/abs_smart_collection_store.go +++ b/internal/audiobooks/abs_smart_collection_store.go @@ -22,8 +22,10 @@ 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 -// schema, so the empty string stands in for "primary profile". +// profile_id is a text column in the canonical schema, NOT NULL and +// defaulting to the empty string, which stands in for "primary profile". +// (Spelling that default as a pair of SQL quotes here would not survive +// gofmt, which folds them into a typographic quote in a doc comment.) // // abs.SmartCollection.Color and abs.SmartCollection.IsPinned have no // canonical columns (deferred per spec §6); reads always return the zero diff --git a/internal/settingscontract/load.go b/internal/settingscontract/load.go index 1d4835f8..f26cb696 100644 --- a/internal/settingscontract/load.go +++ b/internal/settingscontract/load.go @@ -6,6 +6,7 @@ import ( "fmt" "io/fs" "path" + "strings" "sync" "github.com/santhosh-tekuri/jsonschema/v6" @@ -188,6 +189,12 @@ func compileObjectSchemas(fsys fs.FS) (map[string][]byte, map[string]*jsonschema continue } name := entry.Name() + // schema_ref can only name a .json file, so anything else here is not a + // value schema. Parsing it anyway would turn a stray file — an editor + // backup, a .DS_Store — into a panic at startup via MustLoad. + if !strings.HasSuffix(name, ".json") { + continue + } body, err := fs.ReadFile(fsys, path.Join(schemasDir, name)) if err != nil { return nil, nil, fmt.Errorf("reading value schema %s: %w", name, err) diff --git a/web/src/hooks/themePreferences.ts b/web/src/hooks/themePreferences.ts index 2f0cc72e..51c297e1 100644 --- a/web/src/hooks/themePreferences.ts +++ b/web/src/hooks/themePreferences.ts @@ -1,4 +1,5 @@ import { appearanceCache, storage } from "@/utils/storage"; +import { useOptionalAuth } from "@/hooks/useAuth"; import type { ThemeId } from "@/lib/themes"; import { DEFAULT_THEME, THEME_IDS } from "@/lib/themes"; @@ -26,6 +27,23 @@ export function appearanceCacheOwner({ loading, user }: AppearanceAuth): string return !loading && user ? String(user.id) : null; } +/** + * `appearanceCacheOwner` for the currently authenticated session. + * + * The three appearance providers each need the same owner token, and each was + * adapting the auth context to `AppearanceAuth` itself with identical code. + * That put the shape of auth back in three places, which is the duplication the + * doc comment above argues against: widening ownership has to be a change to + * this file alone. + */ +export function useAppearanceCacheOwner(): string | null { + const auth = useOptionalAuth(); + return appearanceCacheOwner({ + loading: auth?.loading ?? false, + user: auth?.user ? { id: auth.user.id } : null, + }); +} + export function isValidTheme(value: string | null | undefined): value is ThemeId { return typeof value === "string" && (THEME_IDS as readonly string[]).includes(value); } diff --git a/web/src/hooks/useCustomTheme.ts b/web/src/hooks/useCustomTheme.ts index 93f28617..786c3b3c 100644 --- a/web/src/hooks/useCustomTheme.ts +++ b/web/src/hooks/useCustomTheme.ts @@ -1,7 +1,6 @@ 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 { useAppearanceCacheOwner } from "@/hooks/themePreferences"; import { appearanceCache, storage } from "@/utils/storage"; import { parseVarsJson } from "@/lib/themeExport"; import { sanitizeCss } from "@/lib/cssSanitizer"; @@ -29,13 +28,9 @@ interface UseCustomThemeResult { } export function useCustomTheme(): UseCustomThemeResult { - const auth = useOptionalAuth(); // Owner of the localStorage warm start; null while auth bootstraps or when // nobody is signed in, which keeps the last look on the login screen. - const cacheOwner = appearanceCacheOwner({ - loading: auth?.loading ?? false, - user: auth?.user ? { id: auth.user.id } : null, - }); + const cacheOwner = useAppearanceCacheOwner(); const loadApi = cacheOwner !== null; // API values diff --git a/web/src/hooks/useDateTimeFormat.tsx b/web/src/hooks/useDateTimeFormat.tsx index 61a40f8b..793d67a7 100644 --- a/web/src/hooks/useDateTimeFormat.tsx +++ b/web/src/hooks/useDateTimeFormat.tsx @@ -14,8 +14,7 @@ import type { TimeFormatPreference, } from "@/lib/datetime"; import { useSettings, useSetSetting } from "@/hooks/queries/settings"; -import { useOptionalAuth } from "@/hooks/useAuth"; -import { appearanceCacheOwner } from "@/hooks/themePreferences"; +import { useAppearanceCacheOwner } from "@/hooks/themePreferences"; import { appearanceCache, storage } from "@/utils/storage"; export const DATE_FORMAT_SETTING_KEY = "ui.date_format"; @@ -49,13 +48,9 @@ const DateTimeFormatContext = createContext(n * lib/datetime, and exposes setters for the settings UI. */ export function DateTimeFormatProvider({ children }: { children: ReactNode }) { - const auth = useOptionalAuth(); - // Same owner token as the theme caches, from the same function, so widening + // Same owner token as the theme caches, from the same hook, 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 cacheOwner = useAppearanceCacheOwner(); const loadApiSettings = cacheOwner !== null; const { data: apiSettings } = useSettings({ enabled: loadApiSettings }); const settingMutation = useSetSetting(); diff --git a/web/src/hooks/useTheme.test.ts b/web/src/hooks/useTheme.test.ts index 926e7ed6..eb81fa11 100644 --- a/web/src/hooks/useTheme.test.ts +++ b/web/src/hooks/useTheme.test.ts @@ -22,7 +22,11 @@ describe("appearanceCacheOwner", () => { describe("getInitialTheme", () => { beforeEach(() => { - Object.values(storage.KEYS).forEach((key) => storage.remove(key)); + // Not storage.remove over storage.KEYS: appearanceCache writes namespaced + // keys ("silo-theme:1") and an owner pointer, none of which appear in + // storage.KEYS, so that cleanup left both behind and made these cases + // order-dependent. storage.test.ts already clears the whole store. + localStorage.clear(); }); it("warms up from the cache while the owner is unknown", () => { diff --git a/web/src/hooks/useTheme.tsx b/web/src/hooks/useTheme.tsx index 369a04cc..cdb8eb9a 100644 --- a/web/src/hooks/useTheme.tsx +++ b/web/src/hooks/useTheme.tsx @@ -2,16 +2,15 @@ import { createContext, useContext, useEffect, useState, useCallback } from "rea import type { ReactNode } from "react"; import type { ThemeId } from "@/lib/themes"; import { useSettings, useSetSetting } from "@/hooks/queries/settings"; -import { useOptionalAuth } from "@/hooks/useAuth"; import { useBranding } from "@/hooks/useBranding"; import { appearanceCache, storage } from "@/utils/storage"; import { - appearanceCacheOwner, getInitialTheme, isValidTheme, parseHighContrast, parseTextScale, parseTextWeight, + useAppearanceCacheOwner, } from "@/hooks/themePreferences"; import type { TextScale, TextWeight } from "@/hooks/themePreferences"; @@ -47,14 +46,10 @@ function applyHighContrastToDOM(value: boolean): void { } export function ThemeProvider({ children }: { children: ReactNode }) { - const auth = useOptionalAuth(); // The account that owns the localStorage warm start. Null while auth is // bootstrapping or nobody is signed in, which still trusts the cache so the // app paints in the last look this device used. - const cacheOwner = appearanceCacheOwner({ - loading: auth?.loading ?? false, - user: auth?.user ? { id: auth.user.id } : null, - }); + const cacheOwner = useAppearanceCacheOwner(); const loadApiTheme = cacheOwner !== null; const [themePreference, setThemePreference] = useState(() =>