From 90224c583fb87b2784f9fd298409bd0c757d2e2d Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:40:04 +0000 Subject: [PATCH] fix(settings): clear the remaining review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Small, unrelated except that each was raised on #479. compileObjectSchemas parsed every non-directory file under schemas/ as a JSON Schema, so a stray editor backup or .DS_Store would panic the server at startup through MustLoad. schema_ref can only name a .json file; anything else is skipped. cmd/silo used MustLoad while the ETag check beside it and every other startup failure use log.Fatalf. It now fails the same way, so a bad contract prints an error instead of a stack trace. TestRegistryDefaultsMatchTheContract called scalarDefault before handling null, and scalarDefault rejects null as non-scalar — so the subtest skipped and the comparison after it was unreachable. A nullable contract default could disagree with a non-empty registry default and nothing failed. Confirmed by injecting that drift, which now reports it. The three appearance providers each adapted the auth context to AppearanceAuth with identical code, putting the shape of auth back in three places that widening cache ownership would have to find. useAppearanceCacheOwner now does it once. useTheme.test.ts cleared storage.KEYS between cases, but appearanceCache writes namespaced keys and an owner pointer that are not in that list, so both survived and the suite was order-dependent. It clears the store, as storage.test.ts already did. The abs_smart_collection_store comment is reworded rather than given back its SQL quotes: gofmt folds a pair of apostrophes in a doc comment into a typographic quote, which is how it became one in the first place. Reported by CodeRabbit review on #479. --- cmd/silo/main.go | 5 +++- .../api/handlers/settings_contract_test.go | 23 ++++++++++++++----- .../audiobooks/abs_smart_collection_store.go | 6 +++-- internal/settingscontract/load.go | 7 ++++++ web/src/hooks/themePreferences.ts | 18 +++++++++++++++ web/src/hooks/useCustomTheme.ts | 9 ++------ web/src/hooks/useDateTimeFormat.tsx | 11 +++------ web/src/hooks/useTheme.test.ts | 6 ++++- web/src/hooks/useTheme.tsx | 9 ++------ 9 files changed, 62 insertions(+), 32 deletions(-) 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(() =>