fix(agent-config): make effect dependencies explicit

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<T> {
+  promise: Promise<T>;
+  resolve: (value: T) => void;
+}
+
+function deferred<T>(): Deferred<T> {
+  let resolve!: (value: T) => void;
+  const promise = new Promise<T>((next) => {
+    resolve = next;
+  });
+  return { promise, resolve };
+}
+
+function ConfigProbe({
+  load,
+  save,
+}: {
+  load: () => Promise<Record<string, unknown>>;
+  save: (config: Record<string, unknown>) => Promise<void>;
+}) {
+  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<Record<string, unknown>>,
+    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<Record<string, unknown>>();
+    const scopeB = deferred<Record<string, unknown>>();
+    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"
+    );
+  });
+});

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<Record<string, unknown>>;
   /** Async fn that persists an updated config record. */
   save: (config: Record<string, unknown>) => Promise<void>;
-  /**
-   * 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<string, unknown>) => 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<Record<string, unknown>>({});
   const [loaded, setLoaded] = useState(false);
   const saveTimerRef = useRef<ReturnType<typeof setTimeout> | 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);
     },
   });
 

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<string, unknown>,
+    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<T> {
+  promise: Promise<T>;
+  resolve: (value: T) => void;
+}
+
+function deferred<T>(): Deferred<T> {
+  let resolve!: (value: T) => void;
+  const promise = new Promise<T>((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");
+  });
+});