fix(chat): address conversation and MCP feedback

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
Mubashir Osmani 2026-10-07 18:35:58 +00:00
parent 740d0435a8
commit 08b21713b3
10 changed files with 198 additions and 32 deletions

View file

@ -4,6 +4,7 @@ import { useChatHistory } from "@/components/chat/useChatHistory";
import ChatConversationPage from "./page";
import { renderWithProviders } from "@/../tests/test-utils";
import type { OnUrlUpdateFunction } from "nuqs/adapters/testing";
import { fetchAvailableModels } from "@/components/llm_calls/fetch_models";
const { mockMakeOpenAIResponsesRequest, shellState } = vi.hoisted(() => ({
mockMakeOpenAIResponsesRequest: vi.fn(),
@ -84,6 +85,73 @@ describe("/ui/chat request metrics", () => {
localStorage.clear();
mockMakeOpenAIResponsesRequest.mockReset();
shellState.storageUnavailable = false;
vi.mocked(fetchAvailableModels).mockResolvedValue([{ model_group: "gpt-5.4-mini" }]);
});
it.each([
{ providers: ["openrouter"], stateless: true },
{ providers: ["openai", "openrouter"], stateless: true },
{ providers: ["openai"], stateless: false },
{ providers: undefined, stateless: false },
])("continues a conversation correctly with providers=$providers", async ({ providers, stateless }) => {
vi.mocked(fetchAvailableModels).mockResolvedValue([{ model_group: "gpt-5.4-mini", providers }]);
mockMakeOpenAIResponsesRequest.mockImplementation(async (...args: unknown[]) => {
await new Promise((resolve) => setTimeout(resolve, 0));
(args[1] as (role: string, delta: string) => void)("assistant", "First answer");
(args[15] as (id: string) => void)("response-first");
});
await sendOneMessage();
expect(await screen.findByText("First answer")).toBeVisible();
fireEvent.change(screen.getByRole("textbox"), {
target: { value: "Follow up" },
});
await waitFor(() => expect(screen.getByRole("button", { name: "Send" })).toBeEnabled());
fireEvent.click(screen.getByRole("button", { name: "Send" }));
await waitFor(() => expect(mockMakeOpenAIResponsesRequest).toHaveBeenCalledTimes(2));
const [first, second] = mockMakeOpenAIResponsesRequest.mock.calls;
expect(first[14]).toBeNull();
expect(second[14]).toBe(stateless ? null : "response-first");
expect(second[0]).toEqual(
stateless
? [
{ role: "user", content: "How much did this cost?" },
{ role: "assistant", content: "First answer" },
{ role: "user", content: "Follow up" },
]
: [{ role: "user", content: "Follow up" }],
);
});
it("resends history instead of reusing another model's response ID after changing models", async () => {
vi.mocked(fetchAvailableModels).mockResolvedValue([
{ model_group: "gpt-5.4-mini", providers: ["openai"] },
{ model_group: "second-model", providers: ["openai"] },
]);
mockMakeOpenAIResponsesRequest.mockImplementation(async (...args: unknown[]) => {
await new Promise((resolve) => setTimeout(resolve, 0));
(args[1] as (role: string, delta: string) => void)("assistant", "First answer");
(args[15] as (id: string) => void)("response-first");
});
await sendOneMessage();
expect(await screen.findByText("First answer")).toBeVisible();
fireEvent.click(screen.getByRole("button", { name: /gpt-5\.4-mini/ }));
fireEvent.click(await screen.findByRole("button", { name: "second-model" }));
fireEvent.change(screen.getByRole("textbox"), { target: { value: "Follow up" } });
await waitFor(() => expect(screen.getByRole("button", { name: "Send" })).toBeEnabled());
fireEvent.click(screen.getByRole("button", { name: "Send" }));
await waitFor(() => expect(mockMakeOpenAIResponsesRequest).toHaveBeenCalledTimes(2));
const second = mockMakeOpenAIResponsesRequest.mock.calls[1];
expect(second[2]).toBe("second-model");
expect(second[14]).toBeNull();
expect(second[0]).toEqual([
{ role: "user", content: "How much did this cost?" },
{ role: "assistant", content: "First answer" },
{ role: "user", content: "Follow up" },
]);
});
it("renders latency, TTFT, token counts and cost reported for the assistant turn", async () => {

View file

@ -71,6 +71,7 @@ export default function ChatConversationPage() {
const [selectedModel, setSelectedModel] = useState<string | null>(null);
const [models, setModels] = useState<string[]>([]);
const [statelessModelGroups, setStatelessModelGroups] = useState<Set<string>>(new Set());
const [isLoadingModels, setIsLoadingModels] = useState(true);
const [modelSelectorOpen, setModelSelectorOpen] = useState(false);
const [modelSearchText, setModelSearchText] = useState("");
@ -99,6 +100,9 @@ export default function ChatConversationPage() {
.then((data) => {
const names = (data || []).map((m: { model_group?: string }) => m.model_group ?? "").filter(Boolean);
setModels(names);
setStatelessModelGroups(
new Set(data.filter((model) => model.providers?.includes("openrouter")).map((model) => model.model_group)),
);
try {
const saved = localStorage.getItem(LOCALSTORAGE_MODEL_KEY);
if (saved && names.includes(saved)) {
@ -126,6 +130,7 @@ export default function ChatConversationPage() {
const selectModel = useCallback((model: string) => {
setSelectedModel(model);
setResponsesSessionId(null);
localStorage.setItem(LOCALSTORAGE_MODEL_KEY, model);
setModelSelectorOpen(false);
setModelSearchText("");
@ -160,12 +165,7 @@ export default function ChatConversationPage() {
setResponsesSessionId(null);
}
// On a normal continuation turn with an active session, the Responses API
// already holds the prior context server-side, so we only pass the new
// user message (sending the full history would double-count it).
//
// On the very first turn (no session yet), we send the full history.
const previousResponseId = historyOverride ? null : responsesSessionId;
const previousResponseId = historyOverride || statelessModelGroups.has(model) ? null : responsesSessionId;
const history: Array<{ role: "user" | "assistant"; content: string }> = historyOverride
? [...historyOverride, { role: "user" as const, content: trimmed }]
@ -260,6 +260,7 @@ export default function ChatConversationPage() {
updateLastAssistantMessage,
isStreaming,
responsesSessionId,
statelessModelGroups,
setConversationIdInUrl,
],
);

View file

@ -59,7 +59,7 @@ describe("ViewSwitcher", () => {
fireEvent.click(button);
});
expect(await screen.findByText("Chat")).toBeInTheDocument();
expect(screen.getByText(/Admins can enable in Settings/i)).toBeInTheDocument();
expect(screen.getByText(/Settings > Admin Settings > UI Settings/i)).toBeInTheDocument();
act(() => {
fireEvent.click(screen.getByText("Chat"));
@ -109,7 +109,7 @@ describe("ViewSwitcher", () => {
fireEvent.click(screen.getByRole("button"));
});
expect(await screen.findByText("Chat")).toBeInTheDocument();
expect(screen.queryByText(/Admins can enable in Settings/i)).not.toBeInTheDocument();
expect(screen.queryByText(/Settings > Admin Settings > UI Settings/i)).not.toBeInTheDocument();
act(() => {
fireEvent.click(screen.getByText("Chat"));
@ -145,7 +145,7 @@ describe("ViewSwitcher", () => {
});
expect(await screen.findByText("Observability")).toBeInTheDocument();
expect(screen.getByText("Chat")).toBeInTheDocument();
expect(screen.getByText(/Admins can enable in Settings/i)).toBeInTheDocument();
expect(screen.getByText(/Settings > Admin Settings > UI Settings/i)).toBeInTheDocument();
act(() => {
fireEvent.click(screen.getByText("Chat"));

View file

@ -66,7 +66,7 @@ export default function ViewSwitcher() {
<div className="flex max-w-[220px] flex-col py-0.5">
<span className="font-medium">Chat</span>
<span className="whitespace-normal text-xs leading-snug text-muted-foreground">
Admins can enable in Settings
Admins can enable in Settings &gt; Admin Settings &gt; UI Settings
</span>
</div>
),

View file

@ -1,5 +1,5 @@
import { renderHook, act } from "@testing-library/react";
import { describe, it, expect, beforeEach } from "vitest";
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
import { useChatHistory } from "./useChatHistory";
describe("useChatHistory", () => {
@ -7,6 +7,32 @@ describe("useChatHistory", () => {
localStorage.clear();
});
afterEach(() => {
vi.unstubAllGlobals();
});
it("creates unique conversation and message IDs without crypto.randomUUID", () => {
vi.stubGlobal("crypto", { getRandomValues: crypto.getRandomValues.bind(crypto) });
const { result } = renderHook(() => useChatHistory(null, "http-user"));
act(() => {
result.current.createConversation("test-model");
});
const conversationId = result.current.currentActiveId!;
act(() => {
result.current.appendMessage(conversationId, { role: "user", content: "Hello" });
result.current.appendMessage(conversationId, { role: "assistant", content: "Hi" });
});
const conversation = result.current.activeConversation!;
const ids = [conversation.id, ...conversation.messages.map((message) => message.id)];
expect(new Set(ids).size).toBe(3);
for (const id of ids) {
expect(id).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i);
}
expect(conversation.messages.map((message) => message.content)).toEqual(["Hello", "Hi"]);
});
it("does not leak conversations between different users on the same browser", () => {
const { result: alice } = renderHook(({ userId }) => useChatHistory(null, userId), {
initialProps: { userId: "alice" },

View file

@ -1,4 +1,5 @@
import { useCallback, useEffect, useState } from "react";
import { v4 as uuidv4 } from "uuid";
import { AssistantMessageUpdate, ChatMessage, Conversation } from "./types";
const STORAGE_KEY_PREFIX = "litellm_chat_history_v1";
@ -106,7 +107,7 @@ export function useChatHistory(
}, [conversations, userId, storageUnavailable]);
const createConversation = useCallback((model: string): string => {
const id = crypto.randomUUID();
const id = uuidv4();
const now = Date.now();
const newConversation: Conversation = {
id,
@ -125,7 +126,7 @@ export function useChatHistory(
const appendMessage = useCallback((conversationId: string, message: Omit<ChatMessage, "id" | "timestamp">) => {
const newMessage: ChatMessage = {
...message,
id: crypto.randomUUID(),
id: uuidv4(),
timestamp: Date.now(),
};

View file

@ -0,0 +1,63 @@
import { fireEvent, render, screen } from "@testing-library/react";
import { describe, expect, it } from "vitest";
import type { MCPEvent } from "@/components/mcp_tools/types";
import MCPEventsDisplay from "./MCPEventsDisplay";
const toolsEvent: MCPEvent = {
type: "response.output_item.done",
item: {
type: "mcp_list_tools",
tools: [{ name: "available_lookup", description: "Look up a document" }],
},
};
const callEvent: MCPEvent = {
type: "response.output_item.done",
item: {
type: "mcp_call",
name: "document_lookup",
arguments: '{"query":"chat"}',
output: "Found the requested document",
},
};
describe("MCPEventsDisplay", () => {
it("collapses discovery while showing tool calls and lets users expand discovery", async () => {
render(<MCPEventsDisplay events={[toolsEvent, callEvent]} />);
const listTools = screen.getByRole("button", { name: "List tools" });
expect(listTools).toHaveAttribute("aria-expanded", "false");
expect(screen.queryByText("available_lookup")).not.toBeInTheDocument();
expect(screen.getByRole("button", { name: "document_lookup" })).toHaveAttribute("aria-expanded", "true");
expect(screen.getByText("Found the requested document")).toBeVisible();
fireEvent.click(listTools);
expect(await screen.findByText("available_lookup")).toBeVisible();
expect(listTools).toHaveAttribute("aria-expanded", "true");
});
it("keeps a tool call expanded when no discovery event is present", () => {
render(<MCPEventsDisplay events={[callEvent]} />);
expect(screen.getByRole("button", { name: "document_lookup" })).toHaveAttribute("aria-expanded", "true");
expect(screen.getByText("Found the requested document")).toBeVisible();
});
it("opens streamed tool calls without overriding the user's collapsed panels", () => {
const { rerender } = render(<MCPEventsDisplay events={[toolsEvent]} />);
rerender(<MCPEventsDisplay events={[toolsEvent, callEvent]} />);
const toolCall = screen.getByRole("button", { name: "document_lookup" });
expect(toolCall).toHaveAttribute("aria-expanded", "true");
fireEvent.click(toolCall);
const nextCall: MCPEvent = {
...callEvent,
item: { ...callEvent.item, name: "next_lookup" },
};
rerender(<MCPEventsDisplay events={[toolsEvent, callEvent, nextCall]} />);
expect(toolCall).toHaveAttribute("aria-expanded", "false");
expect(screen.getByRole("button", { name: "List tools" })).toHaveAttribute("aria-expanded", "false");
expect(screen.getByRole("button", { name: "next_lookup" })).toHaveAttribute("aria-expanded", "true");
});
});

View file

@ -45,13 +45,9 @@ const MCPEventsDisplay: React.FC<MCPEventsDisplayProps> = ({ events, className }
return null;
}
const defaultOpenKeys = new Set<string>(
toolsEvent ? ["list-tools"] : mcpCallEvents.map((_, index) => `mcp-call-${index}`),
);
return (
<div className={cn("mcp-events-display", className)}>
<MCPEventsPanels toolsEvent={toolsEvent} mcpCallEvents={mcpCallEvents} defaultOpenKeys={defaultOpenKeys} />
<MCPEventsPanels toolsEvent={toolsEvent} mcpCallEvents={mcpCallEvents} />
</div>
);
};
@ -59,22 +55,13 @@ const MCPEventsDisplay: React.FC<MCPEventsDisplayProps> = ({ events, className }
interface MCPEventsPanelsProps {
toolsEvent: MCPEvent | undefined;
mcpCallEvents: MCPEvent[];
defaultOpenKeys: Set<string>;
}
function MCPEventsPanels({ toolsEvent, mcpCallEvents, defaultOpenKeys }: MCPEventsPanelsProps) {
const [openKeys, setOpenKeys] = useState<Set<string>>(defaultOpenKeys);
function MCPEventsPanels({ toolsEvent, mcpCallEvents }: MCPEventsPanelsProps) {
const [openPanels, setOpenPanels] = useState<Record<string, boolean>>({});
const toggleKey = (key: string, open: boolean) => {
setOpenKeys((prev) => {
const next = new Set(prev);
if (open) {
next.add(key);
} else {
next.delete(key);
}
return next;
});
setOpenPanels((prev) => ({ ...prev, [key]: open }));
};
return (
@ -86,7 +73,7 @@ function MCPEventsPanels({ toolsEvent, mcpCallEvents, defaultOpenKeys }: MCPEven
<MCPEventPanel
panelKey="list-tools"
title="List tools"
open={openKeys.has("list-tools")}
open={openPanels["list-tools"] ?? false}
onOpenChange={(open) => toggleKey("list-tools", open)}
>
<div>
@ -109,7 +96,7 @@ function MCPEventsPanels({ toolsEvent, mcpCallEvents, defaultOpenKeys }: MCPEven
key={key}
panelKey={key}
title={callEvent.item?.name || "Tool call"}
open={openKeys.has(key)}
open={openPanels[key] ?? true}
onOpenChange={(open) => toggleKey(key, open)}
>
<div>

View file

@ -38,6 +38,21 @@ describe("fetchAvailableModels", () => {
vi.clearAllMocks();
});
it("preserves provider metadata for aliased and mixed-provider model groups", async () => {
modelHubCallMock.mockResolvedValue({
data: [
{ model_group: "alias", providers: ["openrouter"] },
{ model_group: "mixed", providers: ["openai", "openrouter"] },
{ model_group: "unknown", providers: null },
],
});
expect(await fetchAvailableModels("token")).toEqual([
{ model_group: "alias", providers: ["openrouter"] },
{ model_group: "mixed", providers: ["openai", "openrouter"] },
{ model_group: "unknown" },
]);
});
it("carries the reasoning capabilities the model hub reports for each group", async () => {
modelHubCallMock.mockResolvedValue({
data: [

View file

@ -5,6 +5,7 @@ import { modelAvailableCall, modelHubCall } from "@/components/networking";
export interface ModelGroup {
model_group: string;
providers?: string[];
mode?: string;
supports_reasoning?: boolean;
supports_fast_mode?: boolean;
@ -13,6 +14,7 @@ export interface ModelGroup {
interface AvailableModel {
model_group?: string | null;
providers?: string[] | null;
model_name?: string | null;
id?: string | null;
mode?: string | null;
@ -25,6 +27,9 @@ const toModelGroup = (item: AvailableModel): ModelGroup => {
const groupName = (item.model_group || item.id || item.model_name) ?? "";
return {
model_group: groupName,
...(Array.isArray(item.providers) && {
providers: item.providers.filter((provider) => typeof provider === "string"),
}),
...(item.mode && { mode: item.mode }),
...(item.supports_reasoning === true && { supports_reasoning: true }),
...(item.supports_fast_mode === true && { supports_fast_mode: true }),