From 36a5a02d5ce01bf258e0ae23c172d748df3975d6 Mon Sep 17 00:00:00 2001 From: sooraj Date: Fri, 25 Apr 2025 05:26:00 +0530 Subject: [PATCH] feat: display error on selection of invalid model (#594) Up-to-date of #78 Fixes #32 addressed requested changes @tibo-openai :) made sense to me though, previous rationale with passing the state up was assuming there could be a future need to have a shared state with all available models being available to the parent --- .../src/components/chat/terminal-chat.tsx | 17 ++- codex-cli/src/components/model-overlay.tsx | 9 +- .../terminal-chat-model-selection.test.tsx | 130 ++++++++++++++++++ 3 files changed, 153 insertions(+), 3 deletions(-) create mode 100644 codex-cli/tests/terminal-chat-model-selection.test.tsx diff --git a/codex-cli/src/components/chat/terminal-chat.tsx b/codex-cli/src/components/chat/terminal-chat.tsx index e3638ac3..7f59c0b3 100644 --- a/codex-cli/src/components/chat/terminal-chat.tsx +++ b/codex-cli/src/components/chat/terminal-chat.tsx @@ -31,6 +31,7 @@ import DiffOverlay from "../diff-overlay.js"; import HelpOverlay from "../help-overlay.js"; import HistoryOverlay from "../history-overlay.js"; import ModelOverlay from "../model-overlay.js"; +import chalk from "chalk"; import { Box, Text } from "ink"; import { spawn } from "node:child_process"; import OpenAI from "openai"; @@ -575,7 +576,7 @@ export default function TerminalChat({ providers={config.providers} currentProvider={provider} hasLastResponse={Boolean(lastResponseId)} - onSelect={(newModel) => { + onSelect={(allModels, newModel) => { log( "TerminalChat: interruptAgent invoked – calling agent.cancel()", ); @@ -585,6 +586,20 @@ export default function TerminalChat({ agent?.cancel(); setLoading(false); + if (!allModels?.includes(newModel)) { + // eslint-disable-next-line no-console + console.error( + chalk.bold.red( + `Model "${chalk.yellow( + newModel, + )}" is not available for provider "${chalk.yellow( + provider, + )}".`, + ), + ); + return; + } + setModel(newModel); setLastResponseId((prev) => prev && newModel !== model ? null : prev, diff --git a/codex-cli/src/components/model-overlay.tsx b/codex-cli/src/components/model-overlay.tsx index c9dde0e6..28b2575a 100644 --- a/codex-cli/src/components/model-overlay.tsx +++ b/codex-cli/src/components/model-overlay.tsx @@ -19,7 +19,7 @@ type Props = { currentProvider?: string; hasLastResponse: boolean; providers?: Record; - onSelect: (model: string) => void; + onSelect: (allModels: Array, model: string) => void; onSelectProvider?: (provider: string) => void; onExit: () => void; }; @@ -153,7 +153,12 @@ export default function ModelOverlay({ } initialItems={items} currentValue={currentModel} - onSelect={onSelect} + onSelect={() => + onSelect( + items?.map((m) => m.value), + currentModel, + ) + } onExit={onExit} /> ); diff --git a/codex-cli/tests/terminal-chat-model-selection.test.tsx b/codex-cli/tests/terminal-chat-model-selection.test.tsx new file mode 100644 index 00000000..4e2bd5e9 --- /dev/null +++ b/codex-cli/tests/terminal-chat-model-selection.test.tsx @@ -0,0 +1,130 @@ +/* eslint-disable no-console */ +import { renderTui } from "./ui-test-helpers.js"; +import React from "react"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import chalk from "chalk"; +import ModelOverlay from "src/components/model-overlay.js"; + +// Mock the necessary dependencies +vi.mock("../src/utils/logger/log.js", () => ({ + log: vi.fn(), +})); + +vi.mock("chalk", () => ({ + default: { + bold: { + red: vi.fn((msg) => `[bold-red]${msg}[/bold-red]`), + }, + yellow: vi.fn((msg) => `[yellow]${msg}[/yellow]`), + }, +})); + +describe("Model Selection Error Handling", () => { + // Create a console.error spy with proper typing + let consoleErrorSpy: ReturnType; + + beforeEach(() => { + consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + vi.clearAllMocks(); + consoleErrorSpy.mockRestore(); + }); + + it("should display error with chalk formatting when selecting unavailable model", () => { + // Setup + const allModels = ["gpt-4", "gpt-3.5-turbo"]; + const currentModel = "gpt-4"; + const unavailableModel = "gpt-invalid"; + const currentProvider = "openai"; + + renderTui( + { + if (!models?.includes(newModel)) { + console.error( + chalk.bold.red( + `Model "${chalk.yellow( + newModel, + )}" is not available for provider "${chalk.yellow( + currentProvider, + )}".`, + ), + ); + return; + } + }} + onSelectProvider={() => {}} + onExit={() => {}} + />, + ); + + const onSelectHandler = vi.fn((models, newModel) => { + if (!models?.includes(newModel)) { + console.error( + chalk.bold.red( + `Model "${chalk.yellow( + newModel, + )}" is not available for provider "${chalk.yellow( + currentProvider, + )}".`, + ), + ); + return; + } + }); + + onSelectHandler(allModels, unavailableModel); + + expect(consoleErrorSpy).toHaveBeenCalled(); + expect(chalk.bold.red).toHaveBeenCalled(); + expect(chalk.yellow).toHaveBeenCalledWith(unavailableModel); + expect(chalk.yellow).toHaveBeenCalledWith(currentProvider); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + `[bold-red]Model "[yellow]${unavailableModel}[/yellow]" is not available for provider "[yellow]${currentProvider}[/yellow]".[/bold-red]`, + ); + }); + + it("should not proceed with model change when model is unavailable", () => { + const mockSetModel = vi.fn(); + const mockSetLastResponseId = vi.fn(); + const mockSaveConfig = vi.fn(); + const mockSetItems = vi.fn(); + const mockSetOverlayMode = vi.fn(); + + const onSelectHandler = vi.fn((allModels, newModel) => { + if (!allModels?.includes(newModel)) { + console.error( + chalk.bold.red( + `Model "${chalk.yellow( + newModel, + )}" is not available for provider "${chalk.yellow("openai")}".`, + ), + ); + return; + } + + mockSetModel(newModel); + mockSetLastResponseId(null); + mockSaveConfig({}); + mockSetItems((prev: Array) => [...prev, {}]); + mockSetOverlayMode("none"); + }); + + onSelectHandler(["gpt-4", "gpt-3.5-turbo"], "gpt-invalid"); + + expect(mockSetModel).not.toHaveBeenCalled(); + expect(mockSetLastResponseId).not.toHaveBeenCalled(); + expect(mockSaveConfig).not.toHaveBeenCalled(); + expect(mockSetItems).not.toHaveBeenCalled(); + expect(mockSetOverlayMode).not.toHaveBeenCalled(); + + expect(consoleErrorSpy).toHaveBeenCalled(); + }); +});