🤖 fix: gateway toggles + model LRU defaults (#1112)

ibetitsmike · web-flow · commit d1d5a25b4c7e · 2025-12-12T10:35:57.000Z
Fixes three related UX issues:

- Gateway toggles now persist correctly (remove double localStorage
writes).
- Model LRU merge guarantees newly-added defaults (e.g. openai:gpt-5.2)
appear even when the list is full, evicting non-defaults first.
- Settings 'Add model' provider dropdown no longer offers mux-gateway
(plus a defensive guard).

Tests:
- Added hook tests for gateway toggles and LRU default inclusion.

_Generated with `mux`_
diff --git a/src/browser/components/Settings/sections/ModelsSection.tsx b/src/browser/components/Settings/sections/ModelsSection.tsx
@@ -36,6 +36,10 @@ export function ModelsSection() {
   const [newModel, setNewModel] = useState<NewModelForm>({ provider: "", modelId: "" });
   const [editing, setEditing] = useState<EditingState | null>(null);
   const [error, setError] = useState<string | null>(null);
+
+  const selectableProviders = SUPPORTED_PROVIDERS.filter(
+    (provider) => !HIDDEN_PROVIDERS.has(provider)
+  );
   const { defaultModel, setDefaultModel } = useModelLRU();
   const gateway = useGateway();
 
@@ -52,6 +56,11 @@ export function ModelsSection() {
   const handleAddModel = useCallback(() => {
     if (!config || !newModel.provider || !newModel.modelId.trim()) return;
 
+    // mux-gateway is a routing layer, not a provider users should add models under.
+    if (HIDDEN_PROVIDERS.has(newModel.provider)) {
+      setError("Mux Gateway models can't be added directly. Enable Gateway per-model instead.");
+      return;
+    }
     const trimmedModelId = newModel.modelId.trim();
 
     // Check for duplicates
@@ -184,9 +193,9 @@ export function ModelsSection() {
                 <SelectValue placeholder="Provider" />
               </SelectTrigger>
               <SelectContent>
-                {SUPPORTED_PROVIDERS.map((p) => (
-                  <SelectItem key={p} value={p}>
-                    {PROVIDER_DISPLAY_NAMES[p]}
+                {selectableProviders.map((provider) => (
+                  <SelectItem key={provider} value={provider}>
+                    {PROVIDER_DISPLAY_NAMES[provider]}
                   </SelectItem>
                 ))}
               </SelectContent>
diff --git a/src/browser/hooks/useGatewayModels.test.ts b/src/browser/hooks/useGatewayModels.test.ts
@@ -0,0 +1,79 @@
+/**
+ * Tests for useGateway hook
+ *
+ * Key invariant: clicking a gateway toggle should flip the persisted value exactly once.
+ *
+ * Regression being tested:
+ * - The hook previously double-wrote to localStorage (usePersistedState + updatePersistedState),
+ *   causing the value to toggle twice and effectively become a no-op.
+ */
+
+import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test";
+import { act, cleanup, renderHook, waitFor } from "@testing-library/react";
+import { GlobalWindow } from "happy-dom";
+import { useGateway } from "./useGatewayModels";
+
+const useProvidersConfigMock = mock(() => ({
+  config: {
+    "mux-gateway": {
+      couponCodeSet: true,
+    },
+  },
+}));
+
+void mock.module("@/browser/hooks/useProvidersConfig", () => ({
+  useProvidersConfig: useProvidersConfigMock,
+}));
+
+describe("useGateway", () => {
+  beforeEach(() => {
+    globalThis.window = new GlobalWindow() as unknown as Window & typeof globalThis;
+    globalThis.document = globalThis.window.document;
+    globalThis.window.localStorage.clear();
+  });
+
+  afterEach(() => {
+    cleanup();
+    globalThis.window = undefined as unknown as Window & typeof globalThis;
+    globalThis.document = undefined as unknown as Document;
+  });
+
+  test("toggleEnabled flips gateway-enabled once per call", async () => {
+    const { result } = renderHook(() => useGateway());
+
+    await waitFor(() => expect(result.current.isConfigured).toBe(true));
+    expect(result.current.isEnabled).toBe(true);
+
+    act(() => result.current.toggleEnabled());
+
+    expect(result.current.isEnabled).toBe(false);
+    expect(globalThis.window.localStorage.getItem("gateway-enabled")).toBe("false");
+
+    act(() => result.current.toggleEnabled());
+
+    expect(result.current.isEnabled).toBe(true);
+    expect(globalThis.window.localStorage.getItem("gateway-enabled")).toBe("true");
+  });
+
+  test("toggleModelGateway flips gateway-models membership once per call", async () => {
+    const { result } = renderHook(() => useGateway());
+
+    await waitFor(() => expect(result.current.isConfigured).toBe(true));
+
+    const modelId = "openai:gpt-5.2";
+
+    act(() => result.current.toggleModelGateway(modelId));
+
+    expect(result.current.modelUsesGateway(modelId)).toBe(true);
+    expect(JSON.parse(globalThis.window.localStorage.getItem("gateway-models") ?? "[]")).toContain(
+      modelId
+    );
+
+    act(() => result.current.toggleModelGateway(modelId));
+
+    expect(result.current.modelUsesGateway(modelId)).toBe(false);
+    expect(
+      JSON.parse(globalThis.window.localStorage.getItem("gateway-models") ?? "[]")
+    ).not.toContain(modelId);
+  });
+});
diff --git a/src/browser/hooks/useGatewayModels.ts b/src/browser/hooks/useGatewayModels.ts
@@ -165,9 +165,9 @@ export function useGateway(): GatewayState {
   const isActive = isConfigured && isEnabled;
 
   const toggleEnabled = useCallback(() => {
+    // usePersistedState writes to localStorage synchronously.
+    // Avoid double-writes here (which would toggle twice and become a no-op).
     setIsEnabled((prev) => !prev);
-    // Also update localStorage synchronously
-    updatePersistedState<boolean>(GATEWAY_ENABLED_KEY, (prev) => !prev);
   }, [setIsEnabled]);
 
   const modelUsesGateway = useCallback(
@@ -177,15 +177,11 @@ export function useGateway(): GatewayState {
 
   const toggleModelGateway = useCallback(
     (modelId: string) => {
-      // Update React state for UI
+      // usePersistedState writes to localStorage synchronously.
+      // Avoid double-writes here (which would toggle twice and become a no-op).
       setEnabledModels((prev) =>
         prev.includes(modelId) ? prev.filter((m) => m !== modelId) : [...prev, modelId]
       );
-      // Also update localStorage synchronously so toGatewayModel() sees it immediately
-      // (usePersistedState batches writes in microtask, which can cause race conditions)
-      updatePersistedState<string[]>(GATEWAY_MODELS_KEY, (prev) =>
-        prev.includes(modelId) ? prev.filter((m) => m !== modelId) : [...prev, modelId]
-      );
     },
     [setEnabledModels]
   );
diff --git a/src/browser/hooks/useModelLRU.test.ts b/src/browser/hooks/useModelLRU.test.ts
@@ -0,0 +1,77 @@
+/**
+ * Tests for useModelLRU hook
+ *
+ * Key invariant: newly-added DEFAULT_MODELS should show up in the selector even if the
+ * persisted LRU list is already full.
+ */
+
+import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test";
+import { cleanup, renderHook, waitFor } from "@testing-library/react";
+import { GlobalWindow } from "happy-dom";
+import { MODEL_ABBREVIATIONS } from "@/browser/utils/slashCommands/registry";
+import { defaultModel } from "@/common/utils/ai/models";
+import { WORKSPACE_DEFAULTS } from "@/constants/workspaceDefaults";
+import { useModelLRU } from "./useModelLRU";
+
+void mock.module("@/browser/contexts/API", () => ({
+  useAPI: () => ({ api: null }),
+}));
+
+describe("useModelLRU", () => {
+  const MAX_LRU_SIZE = 12;
+
+  beforeEach(() => {
+    globalThis.window = new GlobalWindow() as unknown as Window & typeof globalThis;
+    globalThis.document = globalThis.window.document;
+    globalThis.window.localStorage.clear();
+  });
+
+  afterEach(() => {
+    cleanup();
+    globalThis.window = undefined as unknown as Window & typeof globalThis;
+    globalThis.document = undefined as unknown as Document;
+  });
+
+  test("merges in missing defaults even when LRU is full", async () => {
+    const FALLBACK_MODEL = WORKSPACE_DEFAULTS.model ?? defaultModel;
+    const DEFAULT_MODELS = [
+      FALLBACK_MODEL,
+      ...Array.from(new Set(Object.values(MODEL_ABBREVIATIONS))).filter(
+        (m) => m !== FALLBACK_MODEL
+      ),
+    ].slice(0, MAX_LRU_SIZE);
+
+    // The bug report: openai:gpt-5.2 exists in Settings/KNOWN_MODELS but can be missing in the
+    // chat creation selector when model-lru is already at max size.
+    expect(DEFAULT_MODELS).toContain("openai:gpt-5.2");
+
+    const customModel1 = "openai:totally-custom-model";
+    const customModel2 = "openai:totally-custom-model-2";
+
+    // Create a full list that *doesn't* contain openai:gpt-5.2.
+    const seededBase = DEFAULT_MODELS.filter((m) => m !== "openai:gpt-5.2");
+    const seeded = [...seededBase, customModel1, customModel2].slice(0, MAX_LRU_SIZE);
+
+    expect(seeded).toHaveLength(MAX_LRU_SIZE);
+    expect(seeded).not.toContain("openai:gpt-5.2");
+    expect(seeded).toContain(customModel2);
+
+    globalThis.window.localStorage.setItem("model-lru", JSON.stringify(seeded));
+
+    const { result } = renderHook(() => useModelLRU());
+
+    await waitFor(() => expect(result.current.recentModels).toContain("openai:gpt-5.2"));
+
+    // The newly-added default should be present, and we should stay within the cap.
+    expect(result.current.recentModels.length).toBeLessThanOrEqual(MAX_LRU_SIZE);
+
+    const persisted = JSON.parse(
+      globalThis.window.localStorage.getItem("model-lru") ?? "[]"
+    ) as string[];
+    expect(persisted).toContain("openai:gpt-5.2");
+    expect(persisted.length).toBeLessThanOrEqual(MAX_LRU_SIZE);
+
+    // To make room, we should evict a non-default model rather than dropping the new default.
+    expect(persisted).not.toContain(customModel2);
+  });
+});
diff --git a/src/browser/hooks/useModelLRU.ts b/src/browser/hooks/useModelLRU.ts
@@ -17,6 +17,58 @@ const DEFAULT_MODELS = [
   ...Array.from(new Set(Object.values(MODEL_ABBREVIATIONS))).filter((m) => m !== FALLBACK_MODEL),
 ].slice(0, MAX_LRU_SIZE);
 
+function mergeLRUWithDefaults(prev: string[]): string[] {
+  // Migrate any mux-gateway:provider/model entries to canonical form
+  const migrated = prev.map((m) => migrateGatewayModel(m));
+  // Remove any remaining mux-gateway entries that couldn't be migrated
+  const filtered = migrated.filter((m) => !isGatewayFormat(m));
+
+  // Deduplicate while preserving order (migration might create duplicates)
+  const deduped: string[] = [];
+  const seen = new Set<string>();
+  for (const model of filtered) {
+    if (seen.has(model)) {
+      continue;
+    }
+    seen.add(model);
+    deduped.push(model);
+  }
+
+  // Ensure defaults are present. If the list is already full, evict non-default models
+  // (least-recent from the end) to make room for any newly added defaults.
+  const merged = [...deduped];
+  for (const defaultModel of DEFAULT_MODELS) {
+    if (!merged.includes(defaultModel)) {
+      merged.push(defaultModel);
+    }
+  }
+
+  if (merged.length <= MAX_LRU_SIZE) {
+    return merged;
+  }
+
+  const defaultSet = new Set(DEFAULT_MODELS);
+  while (merged.length > MAX_LRU_SIZE) {
+    let idxToRemove = -1;
+    for (let i = merged.length - 1; i >= 0; i--) {
+      if (!defaultSet.has(merged[i])) {
+        idxToRemove = i;
+        break;
+      }
+    }
+
+    // If everything left is a default model, fall back to truncation (should not happen
+    // since DEFAULT_MODELS is capped at MAX_LRU_SIZE).
+    if (idxToRemove === -1) {
+      return merged.slice(0, MAX_LRU_SIZE);
+    }
+
+    merged.splice(idxToRemove, 1);
+  }
+
+  return merged;
+}
+
 function persistModels(models: string[]): void {
   updatePersistedState(LRU_KEY, models.slice(0, MAX_LRU_SIZE));
 }
@@ -63,27 +115,11 @@ export function useModelLRU() {
     { listener: true }
   );
 
-  // Merge any new defaults from MODEL_ABBREVIATIONS and migrate legacy gateway models (only once on mount)
+  // Merge any new defaults from MODEL_ABBREVIATIONS and migrate legacy gateway models.
+  // This runs once on mount and guarantees new defaults show up even if the persisted list is full.
   useEffect(() => {
-    setRecentModels((prev) => {
-      // Migrate any mux-gateway:provider/model entries to canonical form
-      const migrated = prev.map((m) => migrateGatewayModel(m));
-      // Remove any remaining mux-gateway entries that couldn't be migrated
-      const filtered = migrated.filter((m) => !isGatewayFormat(m));
-      // Deduplicate (migration might create duplicates)
-      const deduped = [...new Set(filtered)];
-
-      // Merge defaults
-      const merged = [...deduped];
-      for (const defaultModel of DEFAULT_MODELS) {
-        if (!merged.includes(defaultModel)) {
-          merged.push(defaultModel);
-        }
-      }
-      return merged.slice(0, MAX_LRU_SIZE);
-    });
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, []); // Only run once on mount
+    setRecentModels((prev) => mergeLRUWithDefaults(prev));
+  }, [setRecentModels]);
 
   // Fetch custom models from providers config
   useEffect(() => {
