Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion web/src/components/SessionControls.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,16 @@ vi.mock("../state/connection", () => ({
newRequestId: () => "r",
authIdentity: authMock,
}));
vi.mock("../state/models", () => ({ fetchModels: vi.fn(), modelCatalog: () => [] }));
const fetchModelsMock = vi.hoisted(() => vi.fn(() => Promise.resolve()));
vi.mock("../state/models", () => ({ fetchModels: fetchModelsMock, modelCatalog: () => [] }));
vi.mock("./SessionExportModal", () => ({ openExportModal: vi.fn() }));

import SessionControls from "./SessionControls";
import {
getSession,
ingestSessionList,
focusSession,
mergeSession,
_resetSessionsForTest,
} from "../state/sessions";
import type { SessionInfo } from "../protocol/types";
Expand Down Expand Up @@ -50,6 +52,7 @@ function mockAuth(providers?: string[]): void {
afterEach(() => {
cleanup();
_resetSessionsForTest();
fetchModelsMock.mockClear();
requestMock.mockReset();
requestMock.mockImplementation(() => Promise.resolve(undefined));
authMock.mockReset();
Expand Down Expand Up @@ -383,3 +386,27 @@ describe("ForkButton", () => {
expect(await findByText(/Cannot fork/)).toBeTruthy();
});
});

describe("ModelPicker — catalog follows the backend", () => {
it("fetches the focused session's backend catalog on mount", () => {
mockAuth(["claude", "codex"]);
ingestSessionList([sess("codex")]);
focusSession("s");
render(() => <SessionControls />);
// Not the daemon default — the SESSION's backend.
expect(fetchModelsMock).toHaveBeenCalledWith("codex");
});

it("refetches when the session's backend switches (the reported bug)", async () => {
mockAuth(["claude", "codex"]);
ingestSessionList([sess("claude")]);
focusSession("s");
render(() => <SessionControls />);
expect(fetchModelsMock).toHaveBeenCalledWith("claude");
fetchModelsMock.mockClear();

// A `/provider` switch arrives as an info_update flipping providerId.
mergeSession({ id: "s", providerId: "codex" });
await waitFor(() => expect(fetchModelsMock).toHaveBeenCalledWith("codex"));
});
});
30 changes: 25 additions & 5 deletions web/src/components/SessionControls.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
* silently doing nothing (or worse, desyncing the local store).
*/

import { Component, For, Show, createEffect, createSignal, onCleanup } from "solid-js";
import { Component, For, Show, createEffect, createSignal, on, onCleanup } from "solid-js";

import {
authIdentity,
Expand Down Expand Up @@ -88,7 +88,11 @@ const SessionControls: Component = () => {
<InterruptButton sessionId={s().id} status={s().status} />
<RotateButton sessionId={s().id} />
<ModePicker sessionId={s().id} current={effectiveMode(s())} />
<ModelPicker sessionId={s().id} current={s().model} />
<ModelPicker
sessionId={s().id}
current={s().model}
provider={s().providerId}
/>
<ProviderPicker sessionId={s().id} current={s().providerId} />
<ForkButton sessionId={s().id} current={s().providerId} />
<ExportButton />
Expand Down Expand Up @@ -256,22 +260,38 @@ const ModePicker: Component<{

// Module-level so `/model` (bare) can open the picker programmatically.
const [modelPickerOpen, setModelPickerOpen] = createSignal(false);
/** Open the focused session's model picker (wired to the bare `/model` slash). */
/** Open the focused session's model picker (wired to the bare `/model` slash).
* Fetches the FOCUSED session's backend catalog — not the daemon default. */
export function openModelPicker(): void {
setModelPickerOpen(true);
void fetchModels();
void fetchModels(focusedSession()?.providerId);
}

const ModelPicker: Component<{
sessionId: string;
current?: string;
provider?: string;
}> = (props) => {
const open = modelPickerOpen;
const setOpen = setModelPickerOpen;
const [custom, setCustom] = createSignal("");
const act = createAction();
let rootEl: HTMLDivElement | undefined;
useDismissable(() => rootEl, open, () => setOpen(false));
Comment thread
saucam marked this conversation as resolved.
// Track the session's backend — the catalog is per-backend, so a
// `/provider` switch (or tabbing to a session on another backend) must
// swap the list (the reported bug: switching to codex kept showing
// claude's models). NOT forced: a backend already fetched live serves
// from cache instantly (no daemon round-trip while navigating sessions),
// and a not-yet-live backend still refetches. This must run regardless of
// whether the picker is open — the `/model` slash and the help modal read
// the same catalog.
createEffect(
on(
() => props.provider,
(provider) => void fetchModels(provider),
),
);
return (
<div class="relative" ref={rootEl}>
<button
Expand All @@ -280,7 +300,7 @@ const ModelPicker: Component<{
const next = !open();
setOpen(next);
act.clearError();
if (next) void fetchModels();
if (next) void fetchModels(props.provider);
}}
class="flex items-center gap-1 rounded border border-border bg-bg px-2 py-1 font-mono uppercase tracking-wider text-fg-muted hover:border-accent/40 hover:text-fg"
title="Switch model (next turn applies)"
Expand Down
127 changes: 127 additions & 0 deletions web/src/state/models.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
// @vitest-environment jsdom
import { describe, it, expect, afterEach, beforeEach, vi } from "vitest";

const requestMock = vi.hoisted(() =>
vi.fn<(msg: unknown, opts?: unknown) => Promise<unknown>>(),
);
vi.mock("./connection", () => ({
getClient: () => ({ request: requestMock }),
newRequestId: () => `r-${Math.random()}`,
}));

import {
fetchModels,
modelCatalog,
modelsLive,
_resetModelsForTest,
} from "./models";
import type { ModelInfo } from "../protocol/types";

function result(provider: string, models: ModelInfo[], live = true) {
return {
type: "models.list.result" as const,
requestId: "x",
provider,
models,
live,
};
}

const CLAUDE: ModelInfo[] = [
{ value: "opus", displayName: "Opus" },
{ value: "sonnet", displayName: "Sonnet" },
];
const CODEX: ModelInfo[] = [{ value: "gpt-5-codex", displayName: "GPT-5 Codex" }];

beforeEach(() => _resetModelsForTest());
afterEach(() => {
requestMock.mockReset();
_resetModelsForTest();
});

describe("fetchModels — per-backend catalog", () => {
it("sends the requested provider on the wire", async () => {
requestMock.mockResolvedValueOnce(result("codex", CODEX));
await fetchModels("codex");
expect(requestMock).toHaveBeenCalledTimes(1);
expect(requestMock.mock.calls[0]![0]).toMatchObject({
type: "models.list",
provider: "codex",
});
expect(modelCatalog().map((m) => m.value)).toEqual(["gpt-5-codex"]);
expect(modelsLive()).toBe(true);
});

it("omits provider when none is given (daemon default)", async () => {
requestMock.mockResolvedValueOnce(result("claude", CLAUDE));
await fetchModels();
expect(requestMock.mock.calls[0]![0]).not.toHaveProperty("provider");
});

it("switching backends swaps the catalog — never shows the old backend's models", async () => {
requestMock.mockResolvedValueOnce(result("claude", CLAUDE));
await fetchModels("claude");
expect(modelCatalog().map((m) => m.value)).toEqual(["opus", "sonnet"]);

// Switch to codex: the claude list must not linger.
requestMock.mockResolvedValueOnce(result("codex", CODEX));
await fetchModels("codex", true);
expect(modelCatalog().map((m) => m.value)).toEqual(["gpt-5-codex"]);
expect(requestMock).toHaveBeenCalledTimes(2);
});

it("serves a cached live list without refetching (unless forced)", async () => {
requestMock.mockResolvedValueOnce(result("claude", CLAUDE));
await fetchModels("claude");
await fetchModels("claude"); // cached + live → no second request
expect(requestMock).toHaveBeenCalledTimes(1);

await fetchModels("claude", true); // force → refetch
// (mockResolvedValueOnce is exhausted; force still issues the request)
expect(requestMock).toHaveBeenCalledTimes(2);
});

it("a not-yet-live backend is refetched (models unknown until first use)", async () => {
requestMock.mockResolvedValueOnce(result("codex", [], false)); // empty + not live
await fetchModels("codex");
expect(modelCatalog()).toEqual([]);
expect(modelsLive()).toBe(false);

requestMock.mockResolvedValueOnce(result("codex", CODEX, true)); // now populated
await fetchModels("codex");
expect(requestMock).toHaveBeenCalledTimes(2);
expect(modelCatalog().map((m) => m.value)).toEqual(["gpt-5-codex"]);
});

it("clears a stale catalog immediately when switching to an unfetched backend", async () => {
requestMock.mockResolvedValueOnce(result("claude", CLAUDE));
await fetchModels("claude");

// Switch to codex; the response is slow — the claude list must clear NOW,
// not after the await resolves.
let resolveCodex!: (v: unknown) => void;
requestMock.mockImplementationOnce(() => new Promise((r) => (resolveCodex = r)));
const pending = fetchModels("codex", true);
expect(modelCatalog()).toEqual([]); // synchronously cleared
resolveCodex(result("codex", CODEX));
await pending;
expect(modelCatalog().map((m) => m.value)).toEqual(["gpt-5-codex"]);
});

it("a slow response for a backend the user already switched away from is dropped", async () => {
// codex fetch is slow…
let resolveCodex!: (v: unknown) => void;
requestMock.mockImplementationOnce(() => new Promise((r) => (resolveCodex = r)));
const codexFetch = fetchModels("codex", true);

// …user switches to claude, which resolves first.
requestMock.mockResolvedValueOnce(result("claude", CLAUDE));
await fetchModels("claude", true);
expect(modelCatalog().map((m) => m.value)).toEqual(["opus", "sonnet"]);

// Now the stale codex response lands — it must NOT clobber the claude view.
resolveCodex(result("codex", CODEX));
await codexFetch;
expect(modelCatalog().map((m) => m.value)).toEqual(["opus", "sonnet"]);
});
});
49 changes: 40 additions & 9 deletions web/src/state/models.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,54 @@ export const modelCatalog = models;
/** True once the catalog reflects the live backend list, not a fallback. */
export const modelsLive = live;

let fetched = false;
/** Catalogs are per-backend ("opus" means nothing to codex), so cache by
* provider and remember which one the visible catalog currently reflects —
* a backend switch must refetch, never show the old backend's models. */
const cache = new Map<string, { models: ModelInfo[]; live: boolean }>();
const DEFAULT_KEY = "__default__";
let catalogProvider = DEFAULT_KEY;

/**
* Fetch the model catalog for a backend and make it the visible catalog.
* Pass the focused session's `providerId`; omit it only before a session is
* focused (the daemon then serves its default backend). Cached live lists are
* served instantly; a not-yet-live backend is refetched. `force` refetches
* even a cached live list (used on an explicit backend switch).
*/
export async function fetchModels(provider?: string, force = false): Promise<void> {
const key = provider ?? DEFAULT_KEY;
catalogProvider = key;

const cached = cache.get(key);
if (cached) {
// Show the cached list immediately (no stale-other-backend flash).
setModels(cached.models);
setLive(cached.live);
if (cached.live && !force) return;
} else {
// Switching to a backend we haven't fetched: clear the previous
// backend's list so the picker never shows the wrong models.
setModels([]);
setLive(false);
}

/** Fetch the model catalog from the daemon. Safe to call repeatedly. */
export async function fetchModels(force = false): Promise<void> {
if (fetched && !force && live()) return;
try {
const id = newRequestId();
const result = await getClient().request<ModelsListResultMsg>(
{ type: "models.list", id },
{ type: "models.list", id, ...(provider ? { provider } : {}) },
{
waitForResult: (m) =>
m.type === "models.list.result" && m.requestId === id ? m : undefined,
timeoutMs: 8_000,
},
);
fetched = true;
setModels(result.models);
setLive(result.live);
cache.set(key, { models: result.models, live: result.live });
// A faster switch may have moved the focus to another backend while we
// awaited — only apply if this backend is still the visible one.
if (catalogProvider === key) {
setModels(result.models);
setLive(result.live);
}
} catch {
// Non-fatal — the prompt/picker fall back to whatever is cached (possibly
// empty), and the daemon still validates /model server-side.
Expand Down Expand Up @@ -66,7 +96,8 @@ export function resolveModelInput(input: string): string | null {
}

export function _resetModelsForTest(): void {
fetched = false;
cache.clear();
catalogProvider = DEFAULT_KEY;
setModels([]);
setLive(false);
}
Loading