diff --git a/AGENTS.md b/AGENTS.md index 3e260512..e1290ee2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -166,6 +166,7 @@ For detailed architecture diagrams and documentation, see [`MESSAGE_ARCHITECTURE - **Avoid hardcoding**: No hardcoded folder names, file patterns, or special-case logic - **Configuration over convention**: If behavior needs to vary, make it configurable, not hardcoded - **Universal patterns**: Solutions should work equally well for any folder structure, naming convention, or content type +- **Referential stability — reuse constants for empty values; cache and validate derived views.** Never return a freshly-allocated `[]` / `{}` for an "empty" slice — define a frozen module-level constant (`Object.freeze(...)`) and return it. For derived or filtered views over a settings slice, cache the result on the class/module and validate it by comparing the source-slice reference (`===`) against the live settings on every call; recompute only on miss. Reason: Jotai derived atoms and React memoization both short-circuit on referential equality, so fresh allocations invalidate every downstream cache on every read. Canonical examples: `EMPTY_PROVIDERS` / `EMPTY_CONFIGURED_MODELS` / `EMPTY_BACKENDS` in `src/settings/model.ts` ### TypeScript diff --git a/src/agentMode/skills/SkillManager.ts b/src/agentMode/skills/SkillManager.ts index e8df6d3f..270270e5 100644 --- a/src/agentMode/skills/SkillManager.ts +++ b/src/agentMode/skills/SkillManager.ts @@ -526,9 +526,7 @@ export class SkillManager { const patchResult = await this.runInternalMutation( () => runUpdateProperties({ skill: activeSkill, patch: req.patch, fs }), (r) => - r.ok && vaultRoot !== null - ? buildUpdatePropertiesExpectations(activeSkill, vaultRoot) - : [] + r.ok && vaultRoot !== null ? buildUpdatePropertiesExpectations(activeSkill, vaultRoot) : [] ); if (!patchResult.ok) { if (newName !== undefined) { diff --git a/src/constants.ts b/src/constants.ts index 459572e5..ddd89095 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -1034,6 +1034,9 @@ export const DEFAULT_SETTINGS: CopilotSettings = { importSkipList: [], }, }, + providers: {}, + configuredModels: [], + backends: {}, }; export const EVENT_NAMES = { diff --git a/src/modelManagement/models/ConfiguredModelRegistry.test.ts b/src/modelManagement/models/ConfiguredModelRegistry.test.ts new file mode 100644 index 00000000..2ac2094f --- /dev/null +++ b/src/modelManagement/models/ConfiguredModelRegistry.test.ts @@ -0,0 +1,198 @@ +/** + * Tests for `ConfiguredModelRegistry`. + * + * Real settings store via `resetSettings` / `setSettings`. No keychain + * or Obsidian APIs are touched. + */ + +import { getSettings, resetSettings } from "@/settings/model"; + +import type { ModelInfo } from "@/modelManagement/types/catalog"; + +import { ConfiguredModelRegistry } from "./ConfiguredModelRegistry"; + +jest.mock("@/logger", () => ({ + logInfo: jest.fn(), + logWarn: jest.fn(), + logError: jest.fn(), +})); + +const PROVIDER_A = "provider-a"; +const PROVIDER_B = "provider-b"; + +function info(id: string, displayName = id): ModelInfo { + return { id, displayName }; +} + +describe("ConfiguredModelRegistry", () => { + let registry: ConfiguredModelRegistry; + + beforeEach(() => { + resetSettings(); + registry = new ConfiguredModelRegistry(); + }); + + it("add() mints id, stamps configuredAt, appends row", async () => { + const before = Date.now(); + const id = await registry.add({ + providerId: PROVIDER_A, + info: info("claude-sonnet-4-5", "Claude Sonnet 4.5"), + }); + expect(typeof id).toBe("string"); + const row = registry.get(id)!; + expect(row.providerId).toBe(PROVIDER_A); + expect(row.info.id).toBe("claude-sonnet-4-5"); + expect(row.configuredAt).toBeGreaterThanOrEqual(before); + }); + + it("add() enforces (providerId, info.id) uniqueness", async () => { + await registry.add({ providerId: PROVIDER_A, info: info("m1") }); + await expect( + registry.add({ providerId: PROVIDER_A, info: info("m1", "different label") }) + ).rejects.toThrow(/already configured/); + // Same wire id under a different provider is allowed. + await expect(registry.add({ providerId: PROVIDER_B, info: info("m1") })).resolves.toBeDefined(); + }); + + it("update() merges info patch and rejects unknown id", async () => { + const id = await registry.add({ + providerId: PROVIDER_A, + info: info("m1", "Original"), + }); + await registry.update(id, { info: { displayName: "Renamed" } }); + expect(registry.get(id)!.info.displayName).toBe("Renamed"); + // Wire id unchanged. + expect(registry.get(id)!.info.id).toBe("m1"); + + await expect(registry.update("nope", { info: { displayName: "x" } })).rejects.toThrow( + /unknown/ + ); + }); + + it("remove() drops the row; idempotent for unknown id", async () => { + const id = await registry.add({ providerId: PROVIDER_A, info: info("m1") }); + await registry.remove(id); + expect(registry.get(id)).toBeUndefined(); + // Idempotent: removing the same id twice is a no-op. + await expect(registry.remove(id)).resolves.toBeUndefined(); + }); + + it("removeByProvider() only touches rows under the target provider", async () => { + const a1 = await registry.add({ providerId: PROVIDER_A, info: info("m1") }); + const a2 = await registry.add({ providerId: PROVIDER_A, info: info("m2") }); + const b1 = await registry.add({ providerId: PROVIDER_B, info: info("m1") }); + await registry.removeByProvider(PROVIDER_A); + expect(registry.get(a1)).toBeUndefined(); + expect(registry.get(a2)).toBeUndefined(); + expect(registry.get(b1)).toBeDefined(); + }); + + it("listByProvider() returns stable references when settings unchanged", async () => { + await registry.add({ providerId: PROVIDER_A, info: info("m1") }); + await registry.add({ providerId: PROVIDER_A, info: info("m2") }); + const r1 = registry.listByProvider(PROVIDER_A); + const r2 = registry.listByProvider(PROVIDER_A); + expect(r1).toBe(r2); + expect(r1.length).toBe(2); + + // An empty filtered view returns the shared empty array. + const e1 = registry.listByProvider(PROVIDER_B); + const e2 = registry.listByProvider(PROVIDER_B); + expect(e1).toBe(e2); + expect(e1.length).toBe(0); + }); + + it("list() reuses a stable empty reference when no rows are configured", () => { + const empty1 = registry.list(); + const empty2 = registry.list(); + expect(empty1).toBe(empty2); + expect(empty1.length).toBe(0); + }); + + it("getByWireId() resolves under the right provider only", async () => { + const a1 = await registry.add({ providerId: PROVIDER_A, info: info("m1") }); + await registry.add({ providerId: PROVIDER_B, info: info("m1") }); + expect(registry.getByWireId(PROVIDER_A, "m1")?.configuredModelId).toBe(a1); + expect(registry.getByWireId(PROVIDER_A, "missing")).toBeUndefined(); + }); + + it("bulkSet() preserves configuredModelId for existing (providerId, info.id) matches", async () => { + const reusedInfo = info("m1", "First"); + const m1Id = await registry.add({ providerId: PROVIDER_A, info: reusedInfo }); + const m2Id = await registry.add({ providerId: PROVIDER_A, info: info("m2", "Second") }); + const m1Row = registry.get(m1Id)!; + const m2Row = registry.get(m2Id)!; + const m1ConfiguredAt = m1Row.configuredAt; + + // Re-set with m1 reused (same info object), m2 dropped, m3 added. + const result = await registry.bulkSet(PROVIDER_A, [reusedInfo, info("m3", "Third")]); + + // m1 preserved -> same id; m3 minted -> new id. + expect(result[0]).toBe(m1Id); + expect(result[1]).not.toBe(m2Id); + expect(registry.get(m2Id)).toBeUndefined(); + // Same `info` object passed back -> row reference reused. + expect(registry.get(m1Id)).toBe(m1Row); + expect(registry.get(m1Id)!.configuredAt).toBe(m1ConfiguredAt); + // Sanity: untouched reference for m2 is no longer in the list. + expect(registry.list()).not.toContain(m2Row); + }); + + it("bulkSet() refreshes info for reused rows while keeping configuredModelId stable", async () => { + const m1Id = await registry.add({ providerId: PROVIDER_A, info: info("m1", "Old name") }); + const m1ConfiguredAt = registry.get(m1Id)!.configuredAt; + + // Caller passes a fresh `info` with updated displayName (e.g. catalog refresh). + const result = await registry.bulkSet(PROVIDER_A, [info("m1", "New name")]); + + // configuredModelId + configuredAt preserved. + expect(result[0]).toBe(m1Id); + expect(registry.get(m1Id)!.configuredAt).toBe(m1ConfiguredAt); + // But the refreshed info lands. + expect(registry.get(m1Id)!.info.displayName).toBe("New name"); + }); + + it("bulkSet() reuses the row reference when the fresh info is structurally equal", async () => { + const m1Id = await registry.add({ providerId: PROVIDER_A, info: info("m1", "Same name") }); + const m1Row = registry.get(m1Id)!; + + // Simulate a catalog refresh: caller builds a fresh `info` object + // with byte-identical content but a different reference. The row + // reference must NOT churn — downstream React/Jotai memoization + // counts on row identity remaining stable for no-op refreshes. + const result = await registry.bulkSet(PROVIDER_A, [info("m1", "Same name")]); + + expect(result[0]).toBe(m1Id); + expect(registry.get(m1Id)).toBe(m1Row); + }); + + it("bulkSet() silently dedupes duplicate info.id entries in the input", async () => { + // Two distinct info objects with the same wire id — must collapse to + // one row to preserve the (providerId, info.id) uniqueness invariant + // that `add()` enforces. + const result = await registry.bulkSet(PROVIDER_A, [info("dup"), info("dup", "shadowed")]); + expect(result).toHaveLength(1); + const rows = registry.listByProvider(PROVIDER_A); + expect(rows).toHaveLength(1); + expect(rows[0].configuredModelId).toBe(result[0]); + // First occurrence wins; later duplicates are dropped. + expect(rows[0].info.displayName).toBe("dup"); + }); + + it("bulkSet() does not touch rows belonging to other providers", async () => { + const a1 = await registry.add({ providerId: PROVIDER_A, info: info("m1") }); + const b1 = await registry.add({ providerId: PROVIDER_B, info: info("m1") }); + await registry.bulkSet(PROVIDER_A, [info("m1"), info("m2")]); + // Provider B's row untouched. + expect(registry.get(b1)).toBeDefined(); + // Provider A's m1 reused. + expect(registry.getByWireId(PROVIDER_A, "m1")?.configuredModelId).toBe(a1); + }); + + it("settings reflect mutations atomically", async () => { + const id = await registry.add({ providerId: PROVIDER_A, info: info("m1") }); + expect(getSettings().configuredModels.find((m) => m.configuredModelId === id)).toBeDefined(); + await registry.remove(id); + expect(getSettings().configuredModels.find((m) => m.configuredModelId === id)).toBeUndefined(); + }); +}); diff --git a/src/modelManagement/models/ConfiguredModelRegistry.ts b/src/modelManagement/models/ConfiguredModelRegistry.ts index 2f23f2a1..9f4b53a1 100644 --- a/src/modelManagement/models/ConfiguredModelRegistry.ts +++ b/src/modelManagement/models/ConfiguredModelRegistry.ts @@ -1,18 +1,45 @@ /** * Source of truth for `ConfiguredModel` rows. * - * Wraps `settings.configuredModels: ConfiguredModel[]` (added by the - * settings-wiring follow-up PR) with typed reads and mutations. - * Uniqueness invariant: `(providerId, info.id)` — enforced on `add()` - * and `bulkSet()` writes. + * Wraps `settings.configuredModels: ConfiguredModel[]` with typed reads + * and mutations. Uniqueness invariant: `(providerId, info.id)` — + * enforced on `add()` and implicit in `bulkSet()` writes. * * React components consume reactive reads through the atoms in * `state/atoms.ts`. This class is for mutations and non-React callers. + * + * Referential stability — read methods cache their result keyed on the + * source-slice reference (`getSettings().configuredModels`). On a cache + * hit (slice unchanged since last call) the same array reference is + * returned. `bulkSet` preserves `configuredModelId` + `configuredAt` + * when `(providerId, info.id)` matches an existing row (so external + * refs like `BackendConfig.enabledModels` don't churn), and reuses the + * row reference verbatim only when the same `info` object is passed + * back in. See AGENTS.md → "Referential stability". */ +import { v4 as uuidv4 } from "uuid"; + +import { getSettings, setSettings } from "@/settings/model"; +import { frozenOr, sliceMemoByKey } from "@/utils/sliceCache"; + +import type { ModelInfo } from "@/modelManagement/types/catalog"; import type { ConfiguredModel } from "@/modelManagement/types/persisted"; +// Frozen empty shared across all reads (both `list()` and filtered +// views) so consumers see a stable reference for the zero case. +const EMPTY_LIST: readonly ConfiguredModel[] = Object.freeze([]); + export class ConfiguredModelRegistry { + // Per-provider cache keyed on the source-slice reference; see + // `@/utils/sliceCache` for the underlying pattern. + readonly #byProvider = sliceMemoByKey((source: readonly ConfiguredModel[], providerId: string) => + frozenOr( + source.filter((m) => m.providerId === providerId), + EMPTY_LIST + ) + ); + /** * No constructor args — `settings.configuredModels` is read/written * through the module-level helpers in `@/settings/model`, not @@ -25,22 +52,25 @@ export class ConfiguredModelRegistry { // ------------------------------------------------------------------------- list(): readonly ConfiguredModel[] { - throw new Error("[modelManagement] ConfiguredModelRegistry.list not implemented yet"); + const source = getSettings().configuredModels; + return source.length === 0 ? EMPTY_LIST : source; } listByProvider(providerId: string): readonly ConfiguredModel[] { - throw new Error("[modelManagement] ConfiguredModelRegistry.listByProvider not implemented yet"); + return this.#byProvider(getSettings().configuredModels, providerId); } get(configuredModelId: string): ConfiguredModel | undefined { - throw new Error("[modelManagement] ConfiguredModelRegistry.get not implemented yet"); + return getSettings().configuredModels.find((m) => m.configuredModelId === configuredModelId); } /** Resolve by the wire-form id under a specific provider. Used when * reconciling external references — e.g. an agent picker rehydrating * `/` strings into rows. */ getByWireId(providerId: string, wireModelId: string): ConfiguredModel | undefined { - throw new Error("[modelManagement] ConfiguredModelRegistry.getByWireId not implemented yet"); + return getSettings().configuredModels.find( + (m) => m.providerId === providerId && m.info.id === wireModelId + ); } // ------------------------------------------------------------------------- @@ -49,22 +79,56 @@ export class ConfiguredModelRegistry { /** Mints `configuredModelId`, stamps `configuredAt`. Throws if the * `(providerId, info.id)` pair already exists. */ - add(input: Omit): Promise { - throw new Error("[modelManagement] ConfiguredModelRegistry.add not implemented yet"); + async add(input: Omit): Promise { + const existing = getSettings().configuredModels; + if (existing.some((m) => m.providerId === input.providerId && m.info.id === input.info.id)) { + throw new Error( + `[modelManagement] ConfiguredModelRegistry.add: ` + + `model "${input.info.id}" already configured for providerId ${input.providerId}` + ); + } + const configuredModelId = uuidv4(); + const row: ConfiguredModel = { + ...input, + configuredModelId, + configuredAt: Date.now(), + }; + setSettings((cur) => ({ + configuredModels: [...cur.configuredModels, row], + })); + return configuredModelId; } /** Patches `info` only — id / providerId / configuredAt are * immutable. Used by `CopilotPlusSetupApi` to refresh model * metadata when Plus updates context limits / pricing. */ - update( - configuredModelId: string, - patch: { info?: Partial } - ): Promise { - throw new Error("[modelManagement] ConfiguredModelRegistry.update not implemented yet"); + async update(configuredModelId: string, patch: { info?: Partial }): Promise { + const existing = getSettings().configuredModels.find( + (m) => m.configuredModelId === configuredModelId + ); + if (!existing) { + throw new Error( + `[modelManagement] ConfiguredModelRegistry.update: unknown configuredModelId ${configuredModelId}` + ); + } + if (!patch.info) return; + const next: ConfiguredModel = { + ...existing, + info: { ...existing.info, ...patch.info }, + }; + setSettings((cur) => ({ + configuredModels: cur.configuredModels.map((m) => + m.configuredModelId === configuredModelId ? next : m + ), + })); } - remove(configuredModelId: string): Promise { - throw new Error("[modelManagement] ConfiguredModelRegistry.remove not implemented yet"); + async remove(configuredModelId: string): Promise { + setSettings((cur) => ({ + configuredModels: cur.configuredModels.filter( + (m) => m.configuredModelId !== configuredModelId + ), + })); } /** @@ -75,15 +139,77 @@ export class ConfiguredModelRegistry { * `BackendConfig.enabledModels` refs don't churn. Returns the * resulting `configuredModelId`s in input order. */ - bulkSet(providerId: string, infos: readonly ConfiguredModel["info"][]): Promise { - throw new Error("[modelManagement] ConfiguredModelRegistry.bulkSet not implemented yet"); + async bulkSet(providerId: string, infos: readonly ModelInfo[]): Promise { + const current = getSettings().configuredModels; + const existingForProvider = new Map(); + for (const m of current) { + if (m.providerId === providerId) existingForProvider.set(m.info.id, m); + } + + const resultIds: string[] = []; + const reusedOrNew: ConfiguredModel[] = []; + // Dedupe within the input: callers must not pass duplicate `info.id` + // entries (the `(providerId, info.id)` invariant `add()` enforces). + // Silently drop later duplicates rather than emit rows that violate + // it. + const seenInfoIds = new Set(); + const now = Date.now(); + for (const info of infos) { + if (seenInfoIds.has(info.id)) continue; + seenInfoIds.add(info.id); + const reused = existingForProvider.get(info.id); + if (reused) { + // Preserve `configuredModelId` + `configuredAt` so external refs + // (BackendConfig.enabledModels) don't churn, but adopt the + // caller-supplied `info` — they may have passed refreshed + // catalog metadata (displayName, context limits, pricing). + // Reuse the row reference verbatim when the persisted info is + // structurally equal to the incoming info, so a catalog refresh + // that rebuilds equal-but-fresh info objects doesn't churn the + // row identity for every downstream memoization site. + reusedOrNew.push(isSameInfo(reused.info, info) ? reused : { ...reused, info }); + resultIds.push(reused.configuredModelId); + } else { + const configuredModelId = uuidv4(); + const row: ConfiguredModel = { + configuredModelId, + providerId, + info, + configuredAt: now, + }; + reusedOrNew.push(row); + resultIds.push(configuredModelId); + } + } + + setSettings((cur) => ({ + configuredModels: [ + ...cur.configuredModels.filter((m) => m.providerId !== providerId), + ...reusedOrNew, + ], + })); + return resultIds; } /** Used by `ModelManagementCoordinator.removeProvider` to drop all * models under a provider during cascade. */ - removeByProvider(providerId: string): Promise { - throw new Error( - "[modelManagement] ConfiguredModelRegistry.removeByProvider not implemented yet" - ); + async removeByProvider(providerId: string): Promise { + setSettings((cur) => ({ + configuredModels: cur.configuredModels.filter((m) => m.providerId !== providerId), + })); } } + +/** + * Structural equality for `ModelInfo`. Cheap stringify is sufficient + * here — the type is a small bag of primitives plus a few flat nested + * objects (`modalities`, `limits`, `cost`) whose key order is stable + * across both the catalog fetcher (writes them in a fixed order) and + * Plus-side refresh paths. A `JSON.stringify` mismatch on equal content + * costs at most a fresh row spread; correctness is preserved either + * way. + */ +function isSameInfo(a: ModelInfo, b: ModelInfo): boolean { + if (a === b) return true; + return JSON.stringify(a) === JSON.stringify(b); +} diff --git a/src/modelManagement/providers/ProviderRegistry.test.ts b/src/modelManagement/providers/ProviderRegistry.test.ts new file mode 100644 index 00000000..b213968c --- /dev/null +++ b/src/modelManagement/providers/ProviderRegistry.test.ts @@ -0,0 +1,272 @@ +/** + * Tests for `ProviderRegistry`. + * + * The keychain is mocked via a fake `SecretStorage` mounted on a fake + * `App.secretStorage`. The settings store is real (via + * `resetSettings` / `setSettings`). + */ + +import { resetSettings, getSettings } from "@/settings/model"; +import { KeychainService } from "@/services/keychainService"; + +import type { ProviderAdapter } from "./adapters/ProviderAdapter"; +import { ProviderAdapterRegistry } from "./adapters/ProviderAdapterRegistry"; +import { ProviderRegistry } from "./ProviderRegistry"; + +import type { App } from "obsidian"; +import { z } from "zod"; + +jest.mock("@/logger", () => ({ + logInfo: jest.fn(), + logWarn: jest.fn(), + logError: jest.fn(), +})); + +type SecretStore = Map; + +function makeFakeApp(): { app: App; secrets: SecretStore } { + const secrets: SecretStore = new Map(); + const app = { + secretStorage: { + setSecret: (id: string, value: string) => { + secrets.set(id, value); + }, + getSecret: (id: string) => (secrets.has(id) ? secrets.get(id)! : null), + listSecrets: () => Array.from(secrets.keys()), + deleteSecret: (id: string) => { + secrets.delete(id); + }, + }, + vault: { + // FileSystemAdapter shape is irrelevant for this test — vaultId + // resolution path falls into the random branch and never touches + // adapter methods after the first generation. + adapter: {}, + }, + } as unknown as App; + return { app, secrets }; +} + +const anthropicStub: ProviderAdapter = { + providerType: "anthropic", + extrasSchema: z.object({}).strict(), + buildLangChainClient: () => { + throw new Error("not used in test"); + }, + verifyCredentials: async () => ({ + ok: true, + message: "stub-ok", + checkedAt: 42, + }), +}; + +describe("ProviderRegistry", () => { + let app: App; + let adapters: ProviderAdapterRegistry; + let registry: ProviderRegistry; + + beforeEach(() => { + resetSettings(); + KeychainService.resetInstance(); + const fake = makeFakeApp(); + app = fake.app; + // Eager init so subsequent KeychainService.getInstance() calls inside + // the registry hit the same singleton. + KeychainService.getInstance(app); + adapters = new ProviderAdapterRegistry(); + adapters.register(anthropicStub); + registry = new ProviderRegistry(app, adapters); + }); + + it("add() mints id, stamps addedAt, persists the row", async () => { + const before = Date.now(); + const id = await registry.add({ + providerType: "anthropic", + displayName: "Anthropic (prod)", + origin: { kind: "byok" }, + }); + expect(typeof id).toBe("string"); + expect(id.length).toBeGreaterThan(0); + + const row = registry.get(id); + expect(row).toBeDefined(); + expect(row?.displayName).toBe("Anthropic (prod)"); + expect(row?.providerType).toBe("anthropic"); + expect(row?.origin).toEqual({ kind: "byok" }); + expect(row?.addedAt).toBeGreaterThanOrEqual(before); + expect(row?.apiKeyKeychainId).toBeNull(); + }); + + it("list() / listByOrigin / listByProviderType return stable references when settings unchanged", async () => { + await registry.add({ + providerType: "anthropic", + displayName: "A", + origin: { kind: "byok" }, + }); + await registry.add({ + providerType: "anthropic", + displayName: "B", + origin: { kind: "agent", agentType: "claude-code" }, + }); + const list1 = registry.list(); + const list2 = registry.list(); + expect(list1).toBe(list2); + + const byok1 = registry.listByOrigin("byok"); + const byok2 = registry.listByOrigin("byok"); + expect(byok1).toBe(byok2); + expect(byok1.length).toBe(1); + + const ant1 = registry.listByProviderType("anthropic"); + const ant2 = registry.listByProviderType("anthropic"); + expect(ant1).toBe(ant2); + expect(ant1.length).toBe(2); + }); + + it("empty filtered views reuse a shared frozen empty array", () => { + const empty1 = registry.listByOrigin("byok"); + const empty2 = registry.listByOrigin("copilot-plus"); + expect(empty1).toBe(empty2); + expect(empty1.length).toBe(0); + }); + + it("update() merges patch and refuses to mutate providerId / addedAt / providerType / origin", async () => { + const id = await registry.add({ + providerType: "anthropic", + displayName: "Original", + origin: { kind: "byok" }, + }); + const originalAddedAt = registry.get(id)!.addedAt; + + // Bypass the typed Omit to verify the runtime guard strips immutable + // fields even when callers shove them in via an untyped object. + // providerType is the adapter-dispatch key and origin determines + // which settings tab owns the row — both must stay pinned to the + // values supplied at creation. + await registry.update(id, { + displayName: "Renamed", + baseUrl: "https://example.test", + ...({ + providerId: "hacked", + addedAt: 1, + providerType: "openai", + origin: { kind: "agent", agentType: "claude-code" }, + } as Record), + }); + const row = registry.get(id)!; + expect(row.displayName).toBe("Renamed"); + expect(row.baseUrl).toBe("https://example.test"); + expect(row.providerId).toBe(id); + expect(row.addedAt).toBe(originalAddedAt); + expect(row.providerType).toBe("anthropic"); + expect(row.origin).toEqual({ kind: "byok" }); + }); + + it("update() throws for unknown providerId", async () => { + await expect(registry.update("nope", { displayName: "x" })).rejects.toThrow(/unknown/); + }); + + it("setApiKey mints apiKeyKeychainId on first call and reuses it on rotation", async () => { + const id = await registry.add({ + providerType: "anthropic", + displayName: "A", + origin: { kind: "byok" }, + }); + expect(registry.get(id)!.apiKeyKeychainId).toBeNull(); + + await registry.setApiKey(id, "sk-first"); + const firstKeychainId = registry.get(id)!.apiKeyKeychainId; + const vaultId = KeychainService.getInstance(app).getVaultId(); + // Vault-namespaced so `KeychainService.clearAllVaultSecrets()` (which + // filters by `copilot-v{vaultId}-`) sweeps these entries. + expect(firstKeychainId).toBe(`copilot-v${vaultId}-provider-${id}`); + expect(await registry.getApiKey(id)).toBe("sk-first"); + + await registry.setApiKey(id, "sk-rotated"); + expect(registry.get(id)!.apiKeyKeychainId).toBe(firstKeychainId); + expect(await registry.getApiKey(id)).toBe("sk-rotated"); + }); + + it("update() ignores attempts to overwrite apiKeyKeychainId", async () => { + const id = await registry.add({ + providerType: "anthropic", + displayName: "A", + origin: { kind: "byok" }, + }); + await registry.setApiKey(id, "sk-real"); + const realKeychainId = registry.get(id)!.apiKeyKeychainId; + expect(realKeychainId).not.toBeNull(); + + // Bypass the typed Omit to verify the runtime strip refuses to move + // the keychain pointer (which would orphan the secret or repoint the + // row at a keychain entry this registry never wrote). + await registry.update(id, { + ...({ apiKeyKeychainId: "copilot-v0-provider-attacker" } as Record), + }); + expect(registry.get(id)!.apiKeyKeychainId).toBe(realKeychainId); + // The real secret is still readable. + expect(await registry.getApiKey(id)).toBe("sk-real"); + }); + + it("getApiKey returns null when the provider has no apiKeyKeychainId", async () => { + const id = await registry.add({ + providerType: "anthropic", + displayName: "Ollama-like", + origin: { kind: "byok" }, + }); + expect(await registry.getApiKey(id)).toBeNull(); + }); + + it("clearApiKey drops the keychain entry and clears the pointer", async () => { + const id = await registry.add({ + providerType: "anthropic", + displayName: "A", + origin: { kind: "byok" }, + }); + await registry.setApiKey(id, "sk-x"); + await registry.clearApiKey(id); + expect(registry.get(id)!.apiKeyKeychainId).toBeNull(); + expect(await registry.getApiKey(id)).toBeNull(); + }); + + it("remove() drops the row and the keychain entry", async () => { + const id = await registry.add({ + providerType: "anthropic", + displayName: "A", + origin: { kind: "byok" }, + }); + await registry.setApiKey(id, "sk-x"); + const keychainId = registry.get(id)!.apiKeyKeychainId!; + await registry.remove(id); + expect(registry.get(id)).toBeUndefined(); + // Verify keychain side cleaned up by reading raw storage. + expect(KeychainService.getInstance(app).getSecretById(keychainId)).toBeNull(); + }); + + it("verify() dispatches to the adapter for the row's providerType", async () => { + const id = await registry.add({ + providerType: "anthropic", + displayName: "A", + origin: { kind: "byok" }, + }); + await registry.setApiKey(id, "sk-x"); + const result = await registry.verify(id); + expect(result.ok).toBe(true); + expect(result.message).toBe("stub-ok"); + }); + + it("verify() throws for unknown providerId", async () => { + await expect(registry.verify("nope")).rejects.toThrow(/unknown/); + }); + + it("settings reflect mutations atomically", async () => { + const id = await registry.add({ + providerType: "anthropic", + displayName: "A", + origin: { kind: "byok" }, + }); + expect(getSettings().providers[id]).toBeDefined(); + await registry.remove(id); + expect(getSettings().providers[id]).toBeUndefined(); + }); +}); diff --git a/src/modelManagement/providers/ProviderRegistry.ts b/src/modelManagement/providers/ProviderRegistry.ts index 6b3ccc39..f46d003b 100644 --- a/src/modelManagement/providers/ProviderRegistry.ts +++ b/src/modelManagement/providers/ProviderRegistry.ts @@ -1,33 +1,77 @@ /** * Source of truth for `Provider` rows. * - * Wraps `settings.providers: Record` (added by - * the settings-wiring follow-up PR) with typed reads, mutations, and - * keychain bridging. React components consume reactive reads through - * the atoms in `state/atoms.ts`; this class is for mutations and for - * non-React callers (the chat-model factory, the setup APIs, the - * coordinator). + * Wraps `settings.providers: Record` with typed + * reads, mutations, and keychain bridging. React components consume + * reactive reads through the atoms in `state/atoms.ts`; this class is + * for mutations and for non-React callers (the chat-model factory, the + * setup APIs, the coordinator). * * Cascade semantics — `remove()` does NOT cascade to ConfiguredModels * or BackendConfigs on its own. Call `ModelManagementCoordinator.removeProvider` * from UI code; the coordinator orchestrates the cross-slice * removal. This method exists for the coordinator's use. + * + * Referential stability — read methods cache their result keyed on the + * source-slice reference (`getSettings().providers`). On a cache hit + * (slice unchanged since last call) the same array reference is + * returned, which is what Jotai derived atoms and React memoization + * rely on. See AGENTS.md → "Referential stability". */ import type { App } from "obsidian"; +import { v4 as uuidv4 } from "uuid"; + +import { logError } from "@/logger"; +import { KeychainService } from "@/services/keychainService"; +import { getSettings, setSettings } from "@/settings/model"; +import { frozenOr, sliceMemo, sliceMemoByKey } from "@/utils/sliceCache"; import type { ProviderType } from "@/modelManagement/types/catalog"; import type { Provider, ProviderOrigin } from "@/modelManagement/types/persisted"; import type { VerificationResult } from "@/modelManagement/types/runtime"; import type { ProviderAdapterRegistry } from "./adapters/ProviderAdapterRegistry"; +// Frozen empty shared across all filtered views so consumers see a +// stable reference even when two distinct filters both yield zero rows. +const EMPTY_LIST: readonly Provider[] = Object.freeze([]); + +/** Format the keychain id for a given providerId. + * + * Reason: vault-namespaced (`copilot-v{vaultId}-...`) so the entry is + * picked up by `KeychainService.clearAllVaultSecrets()`, which scopes + * cleanup by that exact prefix. A flat `copilot-provider-{id}` id would + * silently leak past "Delete All Keys" and vault uninstall. */ +function providerKeychainId(vaultId: string, providerId: string): string { + return `copilot-v${vaultId}-provider-${providerId}`; +} + export class ProviderRegistry { - /** - * Constructor signature is final. Placeholder doesn't store deps - * because all method bodies throw; implementer adds private fields - * (`#app`, `#adapters`) when the implementation lands. - */ - constructor(app: App, adapters: ProviderAdapterRegistry) {} + readonly #app: App; + readonly #adapters: ProviderAdapterRegistry; + + readonly #list = sliceMemo((source: Record) => + frozenOr(Object.values(source), EMPTY_LIST) + ); + readonly #byOrigin = sliceMemoByKey( + (source: Record, kind: ProviderOrigin["kind"]) => + frozenOr( + Object.values(source).filter((p) => p.origin.kind === kind), + EMPTY_LIST + ) + ); + readonly #byType = sliceMemoByKey( + (source: Record, providerType: ProviderType) => + frozenOr( + Object.values(source).filter((p) => p.providerType === providerType), + EMPTY_LIST + ) + ); + + constructor(app: App, adapters: ProviderAdapterRegistry) { + this.#app = app; + this.#adapters = adapters; + } // ------------------------------------------------------------------------- // Reads — synchronous, backed by `settings.providers`. @@ -36,28 +80,28 @@ export class ProviderRegistry { /** All providers. Use the atoms in `state/atoms.ts` for reactive * React reads; this method is for non-React callers. */ list(): readonly Provider[] { - throw new Error("[modelManagement] ProviderRegistry.list not implemented yet"); + return this.#list(getSettings().providers); } get(providerId: string): Provider | undefined { - throw new Error("[modelManagement] ProviderRegistry.get not implemented yet"); + return getSettings().providers[providerId]; } /** Filter helper used by the BYOK tab (origin = "byok"), the agent * setup flows (origin = "agent"), and the Plus sign-in handler * (origin = "copilot-plus"). */ listByOrigin(originKind: ProviderOrigin["kind"]): readonly Provider[] { - throw new Error("[modelManagement] ProviderRegistry.listByOrigin not implemented yet"); + return this.#byOrigin(getSettings().providers, originKind); } /** Used by the agent-setup idempotency check (one * `(agentType, providerType)` row at most). */ listByProviderType(providerType: ProviderType): readonly Provider[] { - throw new Error("[modelManagement] ProviderRegistry.listByProviderType not implemented yet"); + return this.#byType(getSettings().providers, providerType); } // ------------------------------------------------------------------------- - // Mutations — persist via `updateSetting("providers", …)`. + // Mutations — persist via `setSettings` updater form. // ------------------------------------------------------------------------- /** @@ -68,16 +112,80 @@ export class ProviderRegistry { * shape so callers cannot create a row whose pointer references a * keychain entry this code path never wrote. */ - add(input: Omit): Promise { - throw new Error("[modelManagement] ProviderRegistry.add not implemented yet"); + async add(input: Omit): Promise { + const providerId = uuidv4(); + const row: Provider = { + ...input, + providerId, + addedAt: Date.now(), + apiKeyKeychainId: null, + }; + setSettings((cur) => ({ + providers: { ...cur.providers, [providerId]: row }, + })); + return providerId; } - /** Partial update. `providerId` and `addedAt` are immutable. */ - update( + /** Partial update. The following fields are immutable through this + * entry point: + * - `providerId` / `addedAt`: identity & creation time. + * - `apiKeyKeychainId`: owned by `setApiKey` / `clearApiKey`; moving + * it via a generic patch would orphan keychain entries or + * repoint the row at a secret this registry never wrote. + * - `providerType`: the single dispatch field — changing it would + * leave the row's keychain entry and `extras` payload (whose + * shape is `providerType`-specific) pointing at a different + * adapter than the one that originally wrote them. + * - `origin`: the BYOK / agent / Plus discriminator — changing it + * silently moves the row between settings tabs and lifecycle + * owners. + * Create a new provider (and re-add models / re-enter the key) if any + * of these need to change. */ + async update( providerId: string, - patch: Partial> + patch: Partial< + Omit + > ): Promise { - throw new Error("[modelManagement] ProviderRegistry.update not implemented yet"); + const existing = getSettings().providers[providerId]; + if (!existing) { + throw new Error( + `[modelManagement] ProviderRegistry.update: unknown providerId ${providerId}` + ); + } + // Defensive: strip immutable fields if any leaked in at runtime + // (TypeScript's Omit covers callers using the typed shape). + const safePatch = { ...patch } as Record; + delete safePatch.providerId; + delete safePatch.addedAt; + delete safePatch.apiKeyKeychainId; + delete safePatch.providerType; + delete safePatch.origin; + if (Object.keys(safePatch).length === 0) return; + const next: Provider = { ...existing, ...(safePatch as Partial) }; + setSettings((cur) => ({ + providers: { ...cur.providers, [providerId]: next }, + })); + } + + /** Internal: writes `apiKeyKeychainId` on the row. Bypasses the public + * `update()` strip so only the keychain-bridge methods in this class + * can move the pointer. */ + #setApiKeyKeychainId(providerId: string, apiKeyKeychainId: string | null): void { + // Read outside the updater so a row that's been concurrently + // removed (or already carries the same pointer) skips the + // setSettings call entirely — avoids broadcasting a fresh settings + // reference to every subscriber for a no-op write. + const existing = getSettings().providers[providerId]; + if (!existing) return; + if (existing.apiKeyKeychainId === apiKeyKeychainId) return; + setSettings((cur) => { + const current = cur.providers[providerId]; + if (!current) return {}; + return { + providers: { ...cur.providers, [providerId]: { ...current, apiKeyKeychainId } }, + }; + }); } /** @@ -85,8 +193,21 @@ export class ProviderRegistry { * entry. Cross-slice cascade (ConfiguredModels + BackendConfig refs) * is the coordinator's job — see class docstring. */ - remove(providerId: string): Promise { - throw new Error("[modelManagement] ProviderRegistry.remove not implemented yet"); + async remove(providerId: string): Promise { + const existing = getSettings().providers[providerId]; + if (!existing) return; + if (existing.apiKeyKeychainId) { + try { + KeychainService.getInstance(this.#app).deleteSecretById(existing.apiKeyKeychainId); + } catch (err) { + logError(`[modelManagement] ProviderRegistry.remove: failed to clear keychain`, err); + } + } + setSettings((cur) => { + const next = { ...cur.providers }; + delete next[providerId]; + return { providers: next }; + }); } // ------------------------------------------------------------------------- @@ -97,20 +218,48 @@ export class ProviderRegistry { /** Reads the keychain entry referenced by the row's * `apiKeyKeychainId`. Returns `null` for providers that don't take * an API key (Ollama, LMStudio, agent-owned providers). */ - getApiKey(providerId: string): Promise { - throw new Error("[modelManagement] ProviderRegistry.getApiKey not implemented yet"); + async getApiKey(providerId: string): Promise { + const row = getSettings().providers[providerId]; + if (!row || !row.apiKeyKeychainId) return null; + return KeychainService.getInstance(this.#app).getSecretById(row.apiKeyKeychainId); } /** Generates a fresh `apiKeyKeychainId` if the provider doesn't * yet have one; persists the row. Re-calling with a different key * rotates in place (same keychain id, new value). */ - setApiKey(providerId: string, apiKey: string): Promise { - throw new Error("[modelManagement] ProviderRegistry.setApiKey not implemented yet"); + async setApiKey(providerId: string, apiKey: string): Promise { + const row = getSettings().providers[providerId]; + if (!row) { + throw new Error( + `[modelManagement] ProviderRegistry.setApiKey: unknown providerId ${providerId}` + ); + } + const keychain = KeychainService.getInstance(this.#app); + const keychainId = + row.apiKeyKeychainId ?? providerKeychainId(keychain.getVaultId(), providerId); + // Persist the row's pointer BEFORE writing to the keychain so a + // crash (or a keychain write that throws) between the two leaves a + // recoverable dangling pointer (empty keychain → getApiKey returns + // null; clearApiKey / remove still know which id to clean up) + // rather than an orphaned keychain entry that no row points at. + if (row.apiKeyKeychainId !== keychainId) { + this.#setApiKeyKeychainId(providerId, keychainId); + } + keychain.setSecretById(keychainId, apiKey); } /** Drops the keychain entry and clears `apiKeyKeychainId` on the row. */ - clearApiKey(providerId: string): Promise { - throw new Error("[modelManagement] ProviderRegistry.clearApiKey not implemented yet"); + async clearApiKey(providerId: string): Promise { + const row = getSettings().providers[providerId]; + if (!row) return; + if (row.apiKeyKeychainId) { + try { + KeychainService.getInstance(this.#app).deleteSecretById(row.apiKeyKeychainId); + } catch (err) { + logError(`[modelManagement] ProviderRegistry.clearApiKey: failed to delete keychain`, err); + } + this.#setApiKeyKeychainId(providerId, null); + } } // ------------------------------------------------------------------------- @@ -119,11 +268,20 @@ export class ProviderRegistry { /** * Issues an adapter-defined "ping". Returns the verification - * result; does NOT persist it. Persistent - * `lastVerifiedAt` / `lastVerificationError` fields are deferred - * (see data-model spec §10). + * result; does NOT persist it. */ - verify(providerId: string): Promise { - throw new Error("[modelManagement] ProviderRegistry.verify not implemented yet"); + async verify(providerId: string): Promise { + const provider = this.get(providerId); + if (!provider) { + throw new Error( + `[modelManagement] ProviderRegistry.verify: unknown providerId ${providerId}` + ); + } + const apiKey = await this.getApiKey(providerId); + return this.#adapters.verifyCredentials(provider.providerType, { + provider, + apiKey, + extras: provider.extras ?? {}, + }); } } diff --git a/src/modelManagement/state/atoms.ts b/src/modelManagement/state/atoms.ts index b44c4f71..921bda4e 100644 --- a/src/modelManagement/state/atoms.ts +++ b/src/modelManagement/state/atoms.ts @@ -10,16 +10,16 @@ * subscribers use `settingsStore.sub(, listener)`. * * The three persisted slices (`providers`, `configuredModels`, - * `backends`) are not yet typed fields on `CopilotSettings` — the - * settings-wiring follow-up PR adds them. Until then, the local - * helper below reads them with fallbacks so the atoms compile and - * downstream UI components can bind to them today. + * `backends`) live directly on `CopilotSettings` and are backfilled + * with frozen empties by `sanitizeSettings` on load — so derived atoms + * never observe a fresh `{}` / `[]` and Jotai's `===` short-circuit + * holds across reads. See AGENTS.md → "Referential stability". */ import { atom } from "jotai"; import { atomFamily } from "jotai/utils"; -import { settingsAtom, type CopilotSettings } from "@/settings/model"; +import { settingsAtom } from "@/settings/model"; import type { BackendConfig, @@ -30,51 +30,20 @@ import type { } from "@/modelManagement/types/persisted"; import type { EnabledBackendEntry } from "@/modelManagement/types/runtime"; -// Stable empty fallbacks. Allocated once so Jotai's `===` short-circuit -// holds when the underlying slices are absent (pre-settings-wiring) or -// unchanged between settings writes — otherwise every settings write -// would invalidate every derived picker atom. -const EMPTY_PROVIDERS: Readonly> = Object.freeze({}); -const EMPTY_CONFIGURED_MODELS: readonly ConfiguredModel[] = Object.freeze([]); -const EMPTY_BACKENDS: Readonly>> = Object.freeze({}); - -/** - * Reads the three persisted slices with fallbacks. Pre-settings-wiring, - * `CopilotSettings` doesn't declare these fields, so we widen via the - * type intersection below — the cast is intentional and isolated to - * this one helper. - */ -function readSlices(settings: CopilotSettings): { - providers: Readonly>; - configuredModels: readonly ConfiguredModel[]; - backends: Readonly>>; -} { - const widened = settings as CopilotSettings & { - providers?: Record; - configuredModels?: ConfiguredModel[]; - backends?: Partial>; - }; - return { - providers: widened.providers ?? EMPTY_PROVIDERS, - configuredModels: widened.configuredModels ?? EMPTY_CONFIGURED_MODELS, - backends: widened.backends ?? EMPTY_BACKENDS, - }; -} - // ----------------------------------------------------------------------------- // Raw slice atoms — derived directly from settings. // ----------------------------------------------------------------------------- export const providersAtom = atom>>( - (get) => readSlices(get(settingsAtom)).providers + (get) => get(settingsAtom).providers ); export const configuredModelsAtom = atom( - (get) => readSlices(get(settingsAtom)).configuredModels + (get) => get(settingsAtom).configuredModels ); export const backendsAtom = atom>>>( - (get) => readSlices(get(settingsAtom)).backends + (get) => get(settingsAtom).backends ); // ----------------------------------------------------------------------------- diff --git a/src/services/keychainService.ts b/src/services/keychainService.ts index 2086e7de..ae246334 100644 --- a/src/services/keychainService.ts +++ b/src/services/keychainService.ts @@ -291,12 +291,27 @@ export class KeychainService { } } - /** Write a value directly to the keychain using a pre-computed ID. */ + /** Write a value directly to the keychain using a pre-computed ID. + * + * CALLER CONTRACT: `keychainId` must be vault-namespaced — typically + * `copilot-v{vaultId}-...` — so it is swept by + * `clearAllVaultSecrets()` and isolated from other vaults / plugins. + * This method is a low-level bridge; it does not validate the + * prefix because some legacy callers compute their own ids. New + * code should derive the id via `toKeychainId(getVaultId(), …)` or + * an equivalent vault-namespaced helper. */ setSecretById(keychainId: string, value: string): void { this.storage.setSecret(keychainId, value); } - /** Delete a keychain entry by its pre-computed ID. */ + /** Read a value directly from the keychain using a pre-computed ID. + * See `setSecretById` for the caller contract on `keychainId`. */ + getSecretById(keychainId: string): string | null { + return this.storage.getSecret(keychainId); + } + + /** Delete a keychain entry by its pre-computed ID. + * See `setSecretById` for the caller contract on `keychainId`. */ deleteSecretById(keychainId: string): void { this.removeSecret(keychainId); } diff --git a/src/settings/model.ts b/src/settings/model.ts index 9c9bd1a4..ae552b06 100644 --- a/src/settings/model.ts +++ b/src/settings/model.ts @@ -4,6 +4,7 @@ import { v4 as uuidv4 } from "uuid"; import type { ModelSelection } from "@/agentMode"; import { type ChainType } from "@/chainType"; +import type { BackendConfig, BackendType, ConfiguredModel, Provider } from "@/modelManagement"; import { type SortStrategy, isSortStrategy } from "@/utils/recentUsageManager"; import { AGENT_MAX_ITERATIONS_LIMIT, @@ -275,6 +276,12 @@ export interface CopilotSettings { importSkipList?: string[]; }; }; + /** + * Model-management persisted slices. + */ + providers: Record; + configuredModels: ConfiguredModel[]; + backends: Partial>; } /** @@ -351,6 +358,13 @@ export interface OpencodeBackendSettings { export const settingsStore = createStore(); export const settingsAtom = atom(DEFAULT_SETTINGS); +/** + * Frozen empty fallbacks for the model-management persisted slices. + */ +const EMPTY_PROVIDERS = Object.freeze({}) as unknown as Record; +const EMPTY_CONFIGURED_MODELS = Object.freeze([]) as unknown as ConfiguredModel[]; +const EMPTY_BACKENDS = Object.freeze({}) as unknown as Partial>; + /** * Resolve a valid embedding model key for the current settings. * @@ -806,6 +820,24 @@ export function sanitizeSettings(settings: CopilotSettings): CopilotSettings { sanitizedSettings.agentMode = sanitizeAgentMode(sanitizedSettings.agentMode); + if ( + !sanitizedSettings.providers || + typeof sanitizedSettings.providers !== "object" || + Array.isArray(sanitizedSettings.providers) + ) { + sanitizedSettings.providers = EMPTY_PROVIDERS; + } + if (!Array.isArray(sanitizedSettings.configuredModels)) { + sanitizedSettings.configuredModels = EMPTY_CONFIGURED_MODELS; + } + if ( + !sanitizedSettings.backends || + typeof sanitizedSettings.backends !== "object" || + Array.isArray(sanitizedSettings.backends) + ) { + sanitizedSettings.backends = EMPTY_BACKENDS; + } + return sanitizedSettings; } diff --git a/src/utils/sliceCache.test.ts b/src/utils/sliceCache.test.ts new file mode 100644 index 00000000..f0743c64 --- /dev/null +++ b/src/utils/sliceCache.test.ts @@ -0,0 +1,97 @@ +import { frozenOr, sliceMemo, sliceMemoByKey } from "./sliceCache"; + +describe("sliceMemo", () => { + it("returns the same value reference when source reference is unchanged", () => { + const compute = jest.fn((src: { rows: number[] }) => src.rows.map((n) => n * 2)); + const memo = sliceMemo(compute); + const source = { rows: [1, 2, 3] }; + + const a = memo(source); + const b = memo(source); + + expect(a).toBe(b); + expect(compute).toHaveBeenCalledTimes(1); + }); + + it("recomputes when the source reference changes, even if contents are equal", () => { + const compute = jest.fn((src: { rows: number[] }) => [...src.rows]); + const memo = sliceMemo(compute); + + const a = memo({ rows: [1, 2, 3] }); + const b = memo({ rows: [1, 2, 3] }); + + expect(a).not.toBe(b); + expect(compute).toHaveBeenCalledTimes(2); + }); +}); + +describe("sliceMemoByKey", () => { + it("caches independently per key and reuses on source-reference match", () => { + const compute = jest.fn((src: Record, key: string) => src[key] ?? 0); + const memo = sliceMemoByKey(compute); + const source = { a: 1, b: 2 }; + + const a1 = memo(source, "a"); + const b1 = memo(source, "b"); + const a2 = memo(source, "a"); + + expect(a1).toBe(a2); + expect(a1).toBe(1); + expect(b1).toBe(2); + expect(compute).toHaveBeenCalledTimes(2); + }); + + it("recomputes a key when source reference changes", () => { + const compute = jest.fn((src: { rows: number[] }, key: number) => src.rows[key]); + const memo = sliceMemoByKey(compute); + + const v1 = memo({ rows: [10, 20] }, 0); + const v2 = memo({ rows: [10, 20] }, 0); + + expect(v1).toBe(10); + expect(v2).toBe(10); + expect(compute).toHaveBeenCalledTimes(2); + }); + + it("invalidates all keys when the source reference flips", () => { + const compute = jest.fn((src: Record, key: string) => src[key] ?? []); + const memo = sliceMemoByKey(compute); + + const src1 = { x: ["a"], y: ["b"] }; + memo(src1, "x"); + memo(src1, "y"); + expect(compute).toHaveBeenCalledTimes(2); + + const src2 = { x: ["a"], y: ["b"] }; + memo(src2, "x"); + memo(src2, "y"); + expect(compute).toHaveBeenCalledTimes(4); + }); +}); + +describe("frozenOr", () => { + it("returns the provided empty constant when input is empty", () => { + const EMPTY: readonly number[] = Object.freeze([]); + expect(frozenOr([], EMPTY)).toBe(EMPTY); + }); + + it("returns a frozen copy (not the same reference) when input is non-empty", () => { + const EMPTY: readonly number[] = Object.freeze([]); + const input = [1, 2, 3]; + const result = frozenOr(input, EMPTY); + + expect(result).not.toBe(input); + expect(result).toEqual([1, 2, 3]); + expect(Object.isFrozen(result)).toBe(true); + }); + + it("does not mutate or freeze the input array", () => { + const EMPTY: readonly number[] = Object.freeze([]); + const input = [1, 2, 3]; + frozenOr(input, EMPTY); + + expect(Object.isFrozen(input)).toBe(false); + input.push(4); + expect(input).toEqual([1, 2, 3, 4]); + }); +}); diff --git a/src/utils/sliceCache.ts b/src/utils/sliceCache.ts new file mode 100644 index 00000000..38d3557d --- /dev/null +++ b/src/utils/sliceCache.ts @@ -0,0 +1,61 @@ +/** + * Tiny memoization helpers for derived views over a settings slice. + * + * The pattern: given a source slice (e.g. `getSettings().providers`) + * that callers re-read every access, cache the derived value (a + * filtered list, a frozen empty, etc.) keyed on the source-slice + * reference. `setSettings` always swaps the slice reference when it + * writes, so a `===` match guarantees the underlying data is + * unchanged. Returning the same reference on cache hits is what lets + * downstream Jotai derived atoms and React memoization short-circuit. + * + * See AGENTS.md → "Referential stability" for the full rationale. + */ + +/** + * Memoize a single-arg derivation over a settings slice. Returns the + * cached value when the source reference hasn't changed since the + * previous call; recomputes otherwise. + */ +export function sliceMemo(compute: (source: S) => V): (source: S) => V { + let cache: { source: S; value: V } | null = null; + return (source) => { + if (cache && cache.source === source) return cache.value; + const value = compute(source); + cache = { source, value }; + return value; + }; +} + +/** + * Memoize a two-arg derivation `(source, key) → value`. Stores one + * cache entry per distinct `key`, each validated against the source + * reference at read time. + * + * Note: the internal `Map` is unbounded — callers are responsible for + * ensuring `K` has finite cardinality (an enum, a small fixed set) or + * grows at most O(domain size). No LRU. + */ +export function sliceMemoByKey( + compute: (source: S, key: K) => V +): (source: S, key: K) => V { + const cache = new Map(); + return (source, key) => { + const hit = cache.get(key); + if (hit && hit.source === source) return hit.value; + const value = compute(source, key); + cache.set(key, { source, value }); + return value; + }; +} + +/** + * Return the shared `empty` constant when `arr` is empty; otherwise + * a frozen shallow copy of `arr`. The copy step is defensive — callers + * commonly pass `someSettingsArr.filter(...)` (already a fresh array + * that's safe to freeze), but accepting `readonly T[]` means we can't + * assume that, and freezing the settings array itself would be a bug. + */ +export function frozenOr(arr: readonly T[], empty: readonly T[]): readonly T[] { + return arr.length === 0 ? empty : Object.freeze(arr.slice()); +}