Redesign settings modal (All-Hands-AI#3751)

* add advanced options switch * remove logic for custom models * remove extra span * consolidate row * no more toast * remove default subtitle * fix title tags * remove getSettingsDifference * delint * delint * fix select buttons * delete test * fix test * fix test * fix labels * remove model-id tests * fix labels * fix labels * rename var * fix test * fix more tests * change some mocks * more fixes * one more fix * remove some mocks * rename things * one more test * ahhhhh * change required settings * fix test * delint * fix build
jaki300 · Sep 9, 2024 · cb3168d · cb3168d
1 parent 2b7517e
commit cb3168d
Show file tree

Hide file tree

Showing 13 changed files with 139 additions and 439 deletions.
diff --git a/frontend/src/components/modals/base-modal/BaseModal.tsx b/frontend/src/components/modals/base-modal/BaseModal.tsx
@@ -39,7 +39,6 @@ function BaseModal({
       data-testid={testID}
       isOpen={isOpen}
       onOpenChange={onOpenChange}
-      title={title}
       isDismissable={isDismissable}
       backdrop="blur"
       hideCloseButton
@@ -51,14 +50,14 @@ function BaseModal({
           <>
             {title && (
               <ModalHeader className="flex flex-col p-0">
-                <HeaderContent title={title} subtitle={subtitle} />
+                <HeaderContent maintitle={title} subtitle={subtitle} />
               </ModalHeader>
             )}
 
             <ModalBody className={bodyClassName}>{children}</ModalBody>
 
             {actions && actions.length > 0 && (
-              <ModalFooter className="flex-col flex justify-start p-0">
+              <ModalFooter className="flex-row flex justify-start p-0">
                 <FooterContent actions={actions} closeModal={closeModal} />
               </ModalFooter>
             )}

diff --git a/frontend/src/components/modals/base-modal/HeaderContent.tsx b/frontend/src/components/modals/base-modal/HeaderContent.tsx
@@ -1,17 +1,17 @@
 import React from "react";
 
 interface HeaderContentProps {
-  title: string;
+  maintitle: string;
   subtitle?: string;
 }
 
 export function HeaderContent({
-  title,
+  maintitle,
   subtitle = undefined,
 }: HeaderContentProps) {
   return (
     <>
-      <h3>{title}</h3>
+      <h3>{maintitle}</h3>
       {subtitle && (
         <span className="text-neutral-400 text-sm font-light">{subtitle}</span>
       )}

diff --git a/frontend/src/components/modals/settings/ModelSelector.test.tsx b/frontend/src/components/modals/settings/ModelSelector.test.tsx
@@ -29,7 +29,7 @@ describe("ModelSelector", () => {
     const onModelChange = vi.fn();
     render(<ModelSelector models={models} onModelChange={onModelChange} />);
 
-    const selector = screen.getByLabelText("Provider");
+    const selector = screen.getByLabelText("LLM Provider");
     expect(selector).toBeInTheDocument();
 
     await user.click(selector);
@@ -45,10 +45,10 @@ describe("ModelSelector", () => {
     const onModelChange = vi.fn();
     render(<ModelSelector models={models} onModelChange={onModelChange} />);
 
-    const modelSelector = screen.getByLabelText("Model");
+    const modelSelector = screen.getByLabelText("LLM Model");
     expect(modelSelector).toBeDisabled();
 
-    const providerSelector = screen.getByLabelText("Provider");
+    const providerSelector = screen.getByLabelText("LLM Provider");
     await user.click(providerSelector);
 
     const vertexAI = screen.getByText("VertexAI");
@@ -62,13 +62,13 @@ describe("ModelSelector", () => {
     const onModelChange = vi.fn();
     render(<ModelSelector models={models} onModelChange={onModelChange} />);
 
-    const providerSelector = screen.getByLabelText("Provider");
+    const providerSelector = screen.getByLabelText("LLM Provider");
     await user.click(providerSelector);
 
     const azureProvider = screen.getByText("Azure");
     await user.click(azureProvider);
 
-    const modelSelector = screen.getByLabelText("Model");
+    const modelSelector = screen.getByLabelText("LLM Model");
     await user.click(modelSelector);
 
     expect(screen.getByText("ada")).toBeInTheDocument();
@@ -84,42 +84,13 @@ describe("ModelSelector", () => {
     expect(screen.getByText("chat-bison-32k")).toBeInTheDocument();
   });
 
-  it("should display the actual litellm model ID as the user is making the selections", async () => {
-    const user = userEvent.setup();
-    const onModelChange = vi.fn();
-    render(<ModelSelector models={models} onModelChange={onModelChange} />);
-
-    const id = screen.getByTestId("model-id");
-    const providerSelector = screen.getByLabelText("Provider");
-    const modelSelector = screen.getByLabelText("Model");
-
-    expect(id).toHaveTextContent("No model selected");
-
-    await user.click(providerSelector);
-    await user.click(screen.getByText("Azure"));
-
-    expect(id).toHaveTextContent("azure/");
-
-    await user.click(modelSelector);
-    await user.click(screen.getByText("ada"));
-    expect(id).toHaveTextContent("azure/ada");
-
-    await user.click(providerSelector);
-    await user.click(screen.getByText("cohere"));
-    expect(id).toHaveTextContent("cohere.");
-
-    await user.click(modelSelector);
-    await user.click(screen.getByText("command-r-v1:0"));
-    expect(id).toHaveTextContent("cohere.command-r-v1:0");
-  });
-
   it("should call onModelChange when the model is changed", async () => {
     const user = userEvent.setup();
     const onModelChange = vi.fn();
     render(<ModelSelector models={models} onModelChange={onModelChange} />);
 
-    const providerSelector = screen.getByLabelText("Provider");
-    const modelSelector = screen.getByLabelText("Model");
+    const providerSelector = screen.getByLabelText("LLM Provider");
+    const modelSelector = screen.getByLabelText("LLM Model");
 
     await user.click(providerSelector);
     await user.click(screen.getByText("Azure"));
@@ -146,29 +117,6 @@ describe("ModelSelector", () => {
     expect(onModelChange).toHaveBeenCalledWith("cohere.command-r-v1:0");
   });
 
-  it("should clear the model ID when the provider is cleared", async () => {
-    const user = userEvent.setup();
-    const onModelChange = vi.fn();
-    render(<ModelSelector models={models} onModelChange={onModelChange} />);
-
-    const providerSelector = screen.getByLabelText("Provider");
-    const modelSelector = screen.getByLabelText("Model");
-
-    await user.click(providerSelector);
-    await user.click(screen.getByText("Azure"));
-
-    await user.click(modelSelector);
-    await user.click(screen.getByText("ada"));
-
-    expect(screen.getByTestId("model-id")).toHaveTextContent("azure/ada");
-
-    await user.clear(providerSelector);
-
-    expect(screen.getByTestId("model-id")).toHaveTextContent(
-      "No model selected",
-    );
-  });
-
   it("should have a default value if passed", async () => {
     const onModelChange = vi.fn();
     render(
@@ -179,9 +127,8 @@ describe("ModelSelector", () => {
       />,
     );
 
-    expect(screen.getByTestId("model-id")).toHaveTextContent("azure/ada");
-    expect(screen.getByLabelText("Provider")).toHaveValue("Azure");
-    expect(screen.getByLabelText("Model")).toHaveValue("ada");
+    expect(screen.getByLabelText("LLM Provider")).toHaveValue("Azure");
+    expect(screen.getByLabelText("LLM Model")).toHaveValue("ada");
   });
 
   it.todo("should disable provider if isDisabled is true");

diff --git a/frontend/src/components/modals/settings/ModelSelector.tsx b/frontend/src/components/modals/settings/ModelSelector.tsx
@@ -21,7 +21,7 @@ export function ModelSelector({
   onModelChange,
   defaultModel,
 }: ModelSelectorProps) {
-  const [litellmId, setLitellmId] = React.useState<string | null>(null);
+  const [, setLitellmId] = React.useState<string | null>(null);
   const [selectedProvider, setSelectedProvider] = React.useState<string | null>(
     null,
   );
@@ -61,14 +61,10 @@ export function ModelSelector({
 
   return (
     <div data-testid="model-selector" className="flex flex-col gap-2">
-      <span className="text-center italic text-gray-500" data-testid="model-id">
-        {litellmId?.replace("other", "") || "No model selected"}
-      </span>
-
-      <div className="flex flex-col gap-3">
+      <div className="flex flex-row gap-3">
         <Autocomplete
           isDisabled={isDisabled}
-          label="Provider"
+          label="LLM Provider"
           placeholder="Select a provider"
           isClearable={false}
           onSelectionChange={(e) => {
@@ -99,7 +95,7 @@ export function ModelSelector({
         </Autocomplete>
 
         <Autocomplete
-          label="Model"
+          label="LLM Model"
           placeholder="Select a model"
           onSelectionChange={(e) => {
             if (e?.toString()) handleChangeModel(e.toString());