diff --git a/src/modules/MainApp/AgentOrgs/config/osAgent/useAgentConfigBase.test.ts b/src/modules/MainApp/AgentOrgs/config/osAgent/useAgentConfigBase.test.ts new file mode 100644 index 000000000..c3af4d7ca --- /dev/null +++ b/src/modules/MainApp/AgentOrgs/config/osAgent/useAgentConfigBase.test.ts @@ -0,0 +1,121 @@ +// @vitest-environment jsdom +import { act, createElement } from "react"; +import { type Root, createRoot } from "react-dom/client"; +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; + +import { useAgentConfigBase } from "./useAgentConfigBase"; + +const undoStack = vi.hoisted(() => ({ snapshot: vi.fn() })); + +vi.mock("@src/components/Message", () => ({ + default: { error: vi.fn() }, +})); +vi.mock("@src/hooks/ui", () => ({ + useUndoStackWithRestore: () => undoStack, +})); + +interface Deferred { + promise: Promise; + resolve: (value: T) => void; +} + +function deferred(): Deferred { + let resolve!: (value: T) => void; + const promise = new Promise((next) => { + resolve = next; + }); + return { promise, resolve }; +} + +function ConfigProbe({ + load, + save, +}: { + load: () => Promise>; + save: (config: Record) => Promise; +}) { + const state = useAgentConfigBase({ load, save }); + return createElement("output", { + "data-loaded": String(state.loaded), + "data-value": String(state.config.value ?? ""), + }); +} + +describe("useAgentConfigBase load scope", () => { + let container: HTMLDivElement; + let root: Root; + const save = vi.fn().mockResolvedValue(undefined); + const actEnvironment = globalThis as typeof globalThis & { + IS_REACT_ACT_ENVIRONMENT?: boolean; + }; + + beforeAll(() => { + actEnvironment.IS_REACT_ACT_ENVIRONMENT = true; + }); + + beforeEach(() => { + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + vi.clearAllMocks(); + }); + + afterAll(() => { + Reflect.deleteProperty(actEnvironment, "IS_REACT_ACT_ENVIRONMENT"); + }); + + function renderProbe( + load: () => Promise>, + persist = save + ) { + act(() => { + root.render(createElement(ConfigProbe, { load, save: persist })); + }); + } + + it("loads once per callback identity and ignores the superseded scope", async () => { + const scopeA = deferred>(); + const scopeB = deferred>(); + const loadA = vi.fn(() => scopeA.promise); + const loadB = vi.fn(() => scopeB.promise); + + renderProbe(loadA); + expect(loadA).toHaveBeenCalledTimes(1); + + renderProbe(loadA); + expect(loadA).toHaveBeenCalledTimes(1); + + renderProbe(loadB); + expect(loadB).toHaveBeenCalledTimes(1); + + await act(async () => { + scopeB.resolve({ value: "scope-b" }); + await scopeB.promise; + }); + expect(container.querySelector("output")?.getAttribute("data-value")).toBe( + "scope-b" + ); + + await act(async () => { + scopeA.resolve({ value: "stale-scope-a" }); + await scopeA.promise; + }); + expect(container.querySelector("output")?.getAttribute("data-value")).toBe( + "scope-b" + ); + }); +}); diff --git a/src/modules/MainApp/AgentOrgs/config/osAgent/useAgentConfigBase.ts b/src/modules/MainApp/AgentOrgs/config/osAgent/useAgentConfigBase.ts index 2133d48a0..0382ad240 100644 --- a/src/modules/MainApp/AgentOrgs/config/osAgent/useAgentConfigBase.ts +++ b/src/modules/MainApp/AgentOrgs/config/osAgent/useAgentConfigBase.ts @@ -9,8 +9,9 @@ * 3. Register a single cleanup effect that cancels any pending timer. * 4. Wire up `useUndoStackWithRestore` so Cmd-Z / Ctrl-Z reverts. * - * Callers (useOSAgentConfig, useSdeAgentConfig) add their agent-specific - * behaviour (credential checking, path parameterisation, …) on top. + * Callers (useOSAgentConfig, useSdeAgentConfig) provide callbacks whose + * identities encode the configuration scope. A changed `load` callback + * reloads that scope; unrelated renders keep the callback stable. */ import { useCallback, useEffect, useRef, useState } from "react"; @@ -25,19 +26,6 @@ export interface UseAgentConfigBaseOptions { load: () => Promise>; /** Async fn that persists an updated config record. */ save: (config: Record) => Promise; - /** - * Optional callback invoked after the undo-restore path writes back a - * previous config snapshot (e.g. to re-check credentials after a model - * rollback). - */ - onRestore?: (restored: Record) => void; - /** - * Values that, when changed, should trigger a fresh load (same semantics - * as useEffect dependency array). Callers that pass `workspacePath` or - * similar should include it here. Default: `[]` (load once on mount). - */ - // eslint-disable-next-line @typescript-eslint/no-explicit-any - loadDeps?: readonly any[]; } export interface UseAgentConfigBaseReturn { @@ -62,13 +50,13 @@ const DEBOUNCE_MS = 500; export function useAgentConfigBase( options: UseAgentConfigBaseOptions ): UseAgentConfigBaseReturn { - const { load, save, onRestore, loadDeps = [] } = options; + const { load, save } = options; const [config, setConfig] = useState>({}); const [loaded, setLoaded] = useState(false); const saveTimerRef = useRef | null>(null); - // Load on mount (and when loadDeps change, e.g. workspacePath) + // Load on mount and whenever the caller changes configuration scope. useEffect(() => { let cancelled = false; @@ -86,9 +74,7 @@ export function useAgentConfigBase( return () => { cancelled = true; }; - // loadDeps are spread into the effect deps array intentionally - // eslint-disable-next-line react-hooks/exhaustive-deps - }, loadDeps); + }, [load]); // Cleanup pending timer on unmount useEffect(() => { @@ -116,7 +102,6 @@ export function useAgentConfigBase( currentValue: config, onRestore: (prev) => { saveConfig(prev); - onRestore?.(prev); }, }); diff --git a/src/modules/MainApp/AgentOrgs/config/osAgent/useOSAgentConfig.credentials.test.ts b/src/modules/MainApp/AgentOrgs/config/osAgent/useOSAgentConfig.credentials.test.ts new file mode 100644 index 000000000..f9e0f1cea --- /dev/null +++ b/src/modules/MainApp/AgentOrgs/config/osAgent/useOSAgentConfig.credentials.test.ts @@ -0,0 +1,150 @@ +// @vitest-environment jsdom +import { act, createElement } from "react"; +import { type Root, createRoot } from "react-dom/client"; +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; + +import { useOSAgentConfig } from "./useOSAgentConfig"; + +const mocks = vi.hoisted(() => ({ + baseState: { + config: { model: "model-a" } as Record, + loaded: true, + saveConfig: vi.fn(), + updateWithUndo: vi.fn(), + }, + checkKeys: vi.fn(), + getAgentConfig: vi.fn(), + updateAgentConfig: vi.fn(), +})); + +vi.mock("@src/api/tauri/agent", () => ({ + checkKeys: mocks.checkKeys, + getAgentConfig: mocks.getAgentConfig, + updateAgentConfig: mocks.updateAgentConfig, +})); +vi.mock("./useAgentConfigBase", () => ({ + useAgentConfigBase: () => mocks.baseState, +})); + +interface Deferred { + promise: Promise; + resolve: (value: T) => void; +} + +function deferred(): Deferred { + let resolve!: (value: T) => void; + const promise = new Promise((next) => { + resolve = next; + }); + return { promise, resolve }; +} + +function CredentialProbe() { + const state = useOSAgentConfig(); + return createElement("output", { + "data-provider": state.credStatus?.provider ?? "", + }); +} + +describe("useOSAgentConfig credential synchronization", () => { + let container: HTMLDivElement; + let root: Root; + const actEnvironment = globalThis as typeof globalThis & { + IS_REACT_ACT_ENVIRONMENT?: boolean; + }; + + beforeAll(() => { + actEnvironment.IS_REACT_ACT_ENVIRONMENT = true; + }); + + beforeEach(() => { + vi.useFakeTimers(); + mocks.baseState.config = { model: "model-a" }; + mocks.baseState.loaded = true; + mocks.checkKeys.mockReset(); + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + vi.useRealTimers(); + }); + + afterAll(() => { + Reflect.deleteProperty(actEnvironment, "IS_REACT_ACT_ENVIRONMENT"); + }); + + function renderProbe() { + act(() => { + root.render(createElement(CredentialProbe)); + }); + } + + function flushDebounce() { + act(() => { + vi.advanceTimersByTime(300); + }); + } + + it("checks only when the current model changes", () => { + mocks.checkKeys.mockImplementation(() => new Promise(() => {})); + + renderProbe(); + flushDebounce(); + expect(mocks.checkKeys).toHaveBeenLastCalledWith("model-a"); + + mocks.baseState.config = { model: "model-a", temperature: 0.4 }; + renderProbe(); + flushDebounce(); + expect(mocks.checkKeys).toHaveBeenCalledTimes(1); + + mocks.baseState.config = { model: "model-b" }; + renderProbe(); + flushDebounce(); + expect(mocks.checkKeys).toHaveBeenLastCalledWith("model-b"); + expect(mocks.checkKeys).toHaveBeenCalledTimes(2); + }); + + it("ignores a stale credential response after the model changes", async () => { + const modelA = deferred<{ found: boolean; provider: string }>(); + const modelB = deferred<{ found: boolean; provider: string }>(); + mocks.checkKeys.mockImplementation((model: string) => + model === "model-a" ? modelA.promise : modelB.promise + ); + + renderProbe(); + flushDebounce(); + + mocks.baseState.config = { model: "model-b" }; + renderProbe(); + flushDebounce(); + + await act(async () => { + modelB.resolve({ found: true, provider: "provider-b" }); + await modelB.promise; + }); + expect( + container.querySelector("output")?.getAttribute("data-provider") + ).toBe("provider-b"); + + await act(async () => { + modelA.resolve({ found: true, provider: "stale-provider-a" }); + await modelA.promise; + }); + expect( + container.querySelector("output")?.getAttribute("data-provider") + ).toBe("provider-b"); + }); +}); diff --git a/src/modules/MainApp/AgentOrgs/config/osAgent/useOSAgentConfig.ts b/src/modules/MainApp/AgentOrgs/config/osAgent/useOSAgentConfig.ts index b37a51fcf..61d6ab3c2 100644 --- a/src/modules/MainApp/AgentOrgs/config/osAgent/useOSAgentConfig.ts +++ b/src/modules/MainApp/AgentOrgs/config/osAgent/useOSAgentConfig.ts @@ -35,17 +35,26 @@ export interface UseOSAgentConfigReturn { export function useOSAgentConfig(): UseOSAgentConfigReturn { const [credStatus, setCredStatus] = useState(null); const credCheckTimerRef = useRef | null>(null); + const credCheckGenerationRef = useRef(0); const debouncedCheckCredentials = useCallback((model: string) => { + const generation = ++credCheckGenerationRef.current; if (credCheckTimerRef.current) clearTimeout(credCheckTimerRef.current); credCheckTimerRef.current = setTimeout(() => { if (!model) { - setCredStatus(null); + if (credCheckGenerationRef.current === generation) { + setCredStatus(null); + } return; } checkKeys(model) - .then((status) => setCredStatus(status as unknown as CredentialStatus)) + .then((status) => { + if (credCheckGenerationRef.current === generation) { + setCredStatus(status as unknown as CredentialStatus); + } + }) .catch((err) => { + if (credCheckGenerationRef.current !== generation) return; log.warn("[OSAgent] credential check failed:", err); setCredStatus(null); }); @@ -55,38 +64,42 @@ export function useOSAgentConfig(): UseOSAgentConfigReturn { // Cleanup cred-check timer on unmount useEffect(() => { return () => { + credCheckGenerationRef.current += 1; if (credCheckTimerRef.current) clearTimeout(credCheckTimerRef.current); }; }, []); - const { config, loaded, saveConfig, updateWithUndo } = useAgentConfigBase({ - load: () => + const loadConfig = useCallback( + () => getAgentConfig(RUST_AGENT_TYPE.OS).then( (parsed) => parsed as unknown as Record ), - save: (newConfig) => updateAgentConfig(RUST_AGENT_TYPE.OS, newConfig), - onRestore: (prev) => { - const model = getNestedString(prev, "model", ""); - if (model) debouncedCheckCredentials(model); - }, + [] + ); + const persistConfig = useCallback( + (newConfig: Record) => + updateAgentConfig(RUST_AGENT_TYPE.OS, newConfig), + [] + ); + + const { config, loaded, saveConfig, updateWithUndo } = useAgentConfigBase({ + load: loadConfig, + save: persistConfig, }); - // Check credentials once initial load completes + // Credential status synchronizes with the current model, regardless of + // whether it came from initial load, a direct edit, or undo restoration. + const currentModel = loaded ? getNestedString(config, "model", "") : null; useEffect(() => { - if (loaded) { - const model = getNestedString(config, "model", ""); - if (model) debouncedCheckCredentials(model); - } - }, [loaded]); // eslint-disable-line react-hooks/exhaustive-deps + if (currentModel === null) return; + debouncedCheckCredentials(currentModel); + }, [currentModel, debouncedCheckCredentials]); const update = useCallback( (path: string, value: unknown) => { updateWithUndo(setNested(config, path, value)); - if (path === "model" && typeof value === "string") { - debouncedCheckCredentials(value); - } }, - [config, updateWithUndo, debouncedCheckCredentials] + [config, updateWithUndo] ); return { config, loaded, credStatus, update, rawUpdate: saveConfig }; diff --git a/src/modules/MainApp/AgentOrgs/config/sdeAgent/useSdeAgentConfig.dependencies.test.ts b/src/modules/MainApp/AgentOrgs/config/sdeAgent/useSdeAgentConfig.dependencies.test.ts new file mode 100644 index 000000000..669b37e96 --- /dev/null +++ b/src/modules/MainApp/AgentOrgs/config/sdeAgent/useSdeAgentConfig.dependencies.test.ts @@ -0,0 +1,145 @@ +// @vitest-environment jsdom +import { act, createElement } from "react"; +import { type Root, createRoot } from "react-dom/client"; +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; + +import { useSdeAgentConfig } from "./useSdeAgentConfig"; + +const mocks = vi.hoisted(() => ({ + getAgentConfig: vi.fn(), + updateAgentConfig: vi.fn().mockResolvedValue(undefined), + undoStack: { snapshot: vi.fn() }, +})); + +vi.mock("@src/api/tauri/agent", () => ({ + getAgentConfig: mocks.getAgentConfig, + updateAgentConfig: mocks.updateAgentConfig, +})); +vi.mock("@src/components/Message", () => ({ + default: { error: vi.fn() }, +})); +vi.mock("@src/hooks/ui", () => ({ + useUndoStackWithRestore: () => mocks.undoStack, +})); + +interface Deferred { + promise: Promise; + resolve: (value: T) => void; +} + +function deferred(): Deferred { + let resolve!: (value: T) => void; + const promise = new Promise((next) => { + resolve = next; + }); + return { promise, resolve }; +} + +function SdeConfigProbe({ workspacePath }: { workspacePath: string }) { + const state = useSdeAgentConfig(workspacePath); + return createElement( + "div", + null, + createElement("output", { + "data-value": String(state.config.value ?? ""), + }), + createElement( + "button", + { onClick: () => state.update("value", "edited-workspace-b") }, + "Edit" + ) + ); +} + +describe("useSdeAgentConfig dependency scope", () => { + let container: HTMLDivElement; + let root: Root; + const actEnvironment = globalThis as typeof globalThis & { + IS_REACT_ACT_ENVIRONMENT?: boolean; + }; + + beforeAll(() => { + actEnvironment.IS_REACT_ACT_ENVIRONMENT = true; + }); + + beforeEach(() => { + vi.useFakeTimers(); + mocks.getAgentConfig.mockReset(); + mocks.updateAgentConfig.mockClear(); + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + vi.useRealTimers(); + }); + + afterAll(() => { + Reflect.deleteProperty(actEnvironment, "IS_REACT_ACT_ENVIRONMENT"); + }); + + function renderProbe(workspacePath: string) { + act(() => { + root.render(createElement(SdeConfigProbe, { workspacePath })); + }); + } + + it("reloads once for a new workspace and ignores the old response", async () => { + const workspaceA = deferred>(); + const workspaceB = deferred>(); + mocks.getAgentConfig.mockImplementation( + (_agentType: string, workspacePath: string) => + workspacePath === "/workspace/a" + ? workspaceA.promise + : workspaceB.promise + ); + + renderProbe("/workspace/a"); + expect(mocks.getAgentConfig).toHaveBeenCalledTimes(1); + + renderProbe("/workspace/a"); + expect(mocks.getAgentConfig).toHaveBeenCalledTimes(1); + + renderProbe("/workspace/b"); + expect(mocks.getAgentConfig).toHaveBeenCalledTimes(2); + expect(mocks.getAgentConfig.mock.calls[1]?.[1]).toBe("/workspace/b"); + + await act(async () => { + workspaceB.resolve({ value: "workspace-b" }); + await workspaceB.promise; + }); + expect(container.querySelector("output")?.getAttribute("data-value")).toBe( + "workspace-b" + ); + + await act(async () => { + workspaceA.resolve({ value: "stale-workspace-a" }); + await workspaceA.promise; + }); + expect(container.querySelector("output")?.getAttribute("data-value")).toBe( + "workspace-b" + ); + + act(() => container.querySelector("button")?.click()); + await act(async () => { + await vi.advanceTimersByTimeAsync(500); + }); + expect(mocks.updateAgentConfig).toHaveBeenCalledWith( + expect.anything(), + { value: "edited-workspace-b" }, + "/workspace/b" + ); + }); +}); diff --git a/src/modules/MainApp/AgentOrgs/config/sdeAgent/useSdeAgentConfig.ts b/src/modules/MainApp/AgentOrgs/config/sdeAgent/useSdeAgentConfig.ts index 4d176d9e1..aa5aefdba 100644 --- a/src/modules/MainApp/AgentOrgs/config/sdeAgent/useSdeAgentConfig.ts +++ b/src/modules/MainApp/AgentOrgs/config/sdeAgent/useSdeAgentConfig.ts @@ -6,7 +6,7 @@ * * Load / debounced-save / undo wiring is provided by useAgentConfigBase. */ -import { useCallback, useRef } from "react"; +import { useCallback } from "react"; import { getAgentConfig, updateAgentConfig } from "@src/api/tauri/agent"; import { RUST_AGENT_TYPE } from "@src/api/tauri/agent/types"; @@ -23,39 +23,24 @@ export interface UseSdeAgentConfigReturn { export function useSdeAgentConfig( workspacePath?: string ): UseSdeAgentConfigReturn { - // Keep latest workspacePath in a ref so the stable load/save callbacks - // always see the current value without causing dep-array churn. - const workspacePathRef = useRef(workspacePath); - workspacePathRef.current = workspacePath; - const load = useCallback( () => getAgentConfig( RUST_AGENT_TYPE.SDE, - workspacePathRef.current ?? "" + workspacePath ?? "" ) as unknown as Promise>, - // stable — workspacePath changes are handled via workspacePathRef - // eslint-disable-next-line react-hooks/exhaustive-deps - [] + [workspacePath] ); const save = useCallback( (newConfig: Record) => - updateAgentConfig( - RUST_AGENT_TYPE.SDE, - newConfig, - workspacePathRef.current ?? "" - ), - // stable — same reasoning as load - // eslint-disable-next-line react-hooks/exhaustive-deps - [] + updateAgentConfig(RUST_AGENT_TYPE.SDE, newConfig, workspacePath ?? ""), + [workspacePath] ); const { config, loaded, updateWithUndo } = useAgentConfigBase({ load, save, - // Re-fetch when the workspace path changes - loadDeps: [workspacePath], }); // Update a single key (supports dotted paths like "security.autonomy")