Repository: All-Hands-AI/OpenHands
Pull request: feat: set API key from settings modal
depends on https://github.com/OpenDevin/OpenDevin/pull/1291

fix https://github.com/OpenDevin/OpenDevin/issues/1146

Problem statement:
Setting for API key in the UI
**What problem or use case are you trying to solve?**
Let the user paste their API key into the UI

**Describe the UX of the solution you'd like**
A password box in the settings modal. 

It should be saved in local storage, and associated with the selected model.

If the user changes to a new model, we should clear the API key input, but persist the old API key in localstorage

Open question: how do we note that some models use the same API key (e.g. gpt-4 and gpt-3.5)? Should we just have one key for now?

**Do you have thoughts on the technical implementation?**
The API key should be sent via the `initialize` event, and will need to be used when constructing an LLM object.

Hints from issue discussion:
I think the GUI should also prevent users from modifying API key when a task is executing.
@li-boxuan that does sound reasonable! Honestly all settings should probably be frozen while a task is in progress
Being frozen is quite necessary. I may prefer that people first finish the setup first, and then run the whole task (now it seems that everything is still a single turn). We may have some reset button, then users can setup the key and model for the next task.

Now there is another pain is that the key is actually highly correlated with the model. Do we need to have some check to make the key and the model work first before entering the task?
> @li-boxuan that does sound reasonable! Honestly all settings should probably be frozen while a task is in progress

Implemented in https://github.com/OpenDevin/OpenDevin/pull/1291

Commit message: fix settings update

Patch to review:
diff --git a/frontend/src/components/file-explorer/FileExplorer.tsx b/frontend/src/components/file-explorer/FileExplorer.tsx
index f66f1c72a955..89bc6c716ff9 100644
--- a/frontend/src/components/file-explorer/FileExplorer.tsx
+++ b/frontend/src/components/file-explorer/FileExplorer.tsx
@@ -1,4 +1,3 @@
-import { WorkspaceFile, getWorkspace } from "#/services/fileService";
 import React from "react";
 import {
   IoIosArrowBack,
@@ -6,6 +5,7 @@ import {
   IoIosRefresh,
 } from "react-icons/io";
 import { twMerge } from "tailwind-merge";
+import { WorkspaceFile, getWorkspace } from "#/services/fileService";
 import IconButton from "../IconButton";
 import ExplorerTree from "./ExplorerTree";
 import { removeEmptyNodes } from "./utils";
diff --git a/frontend/src/components/modals/settings/SettingsForm.test.tsx b/frontend/src/components/modals/settings/SettingsForm.test.tsx
index 023571a78fce..35c0a4c0110f 100644
--- a/frontend/src/components/modals/settings/SettingsForm.test.tsx
+++ b/frontend/src/components/modals/settings/SettingsForm.test.tsx
@@ -1,14 +1,15 @@
+import { Settings } from "#/services/settings";
+import AgentTaskState from "#/types/AgentTaskState";
 import { act, screen } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
 import React from "react";
 import { renderWithProviders } from "test-utils";
-import AgentTaskState from "#/types/AgentTaskState";
 import SettingsForm from "./SettingsForm";
-import { Settings } from "#/services/settings";
 
 const onModelChangeMock = vi.fn();
 const onAgentChangeMock = vi.fn();
 const onLanguageChangeMock = vi.fn();
+const onAPIKeyChangeMock = vi.fn();
 
 const renderSettingsForm = (settings?: Settings) => {
   renderWithProviders(
@@ -18,6 +19,7 @@ const renderSettingsForm = (settings?: Settings) => {
           LLM_MODEL: "model1",
           AGENT: "agent1",
           LANGUAGE: "en",
+          LLM_API_KEY: "sk-...",
         }
       }
       models={["model1", "model2", "model3"]}
@@ -25,6 +27,7 @@ const renderSettingsForm = (settings?: Settings) => {
       onModelChange={onModelChangeMock}
       onAgentChange={onAgentChangeMock}
       onLanguageChange={onLanguageChangeMock}
+      onAPIKeyChange={onAPIKeyChangeMock}
     />,
   );
 };
@@ -36,10 +39,12 @@ describe("SettingsForm", () => {
     const modelInput = screen.getByRole("combobox", { name: "model" });
     const agentInput = screen.getByRole("combobox", { name: "agent" });
     const languageInput = screen.getByRole("combobox", { name: "language" });
+    const apiKeyInput = screen.getByTestId("apikey");
 
     expect(modelInput).toHaveValue("model1");
     expect(agentInput).toHaveValue("agent1");
     expect(languageInput).toHaveValue("English");
+    expect(apiKeyInput).toHaveValue("sk-...");
   });
 
   it("should display the existing values if it they are present", () => {
@@ -47,6 +52,7 @@ describe("SettingsForm", () => {
       LLM_MODEL: "model2",
       AGENT: "agent2",
       LANGUAGE: "es",
+      LLM_API_KEY: "sk-...",
     });
 
     const modelInput = screen.getByRole("combobox", { name: "model" });
@@ -65,12 +71,14 @@ describe("SettingsForm", () => {
           LLM_MODEL: "model1",
           AGENT: "agent1",
           LANGUAGE: "en",
+          LLM_API_KEY: "sk-...",
         }}
         models={["model1", "model2", "model3"]}
         agents={["agent1", "agent2", "agent3"]}
         onModelChange={onModelChangeMock}
         onAgentChange={onAgentChangeMock}
         onLanguageChange={onLanguageChangeMock}
+        onAPIKeyChange={onAPIKeyChangeMock}
       />,
       { preloadedState: { agent: { curTaskState: AgentTaskState.RUNNING } } },
     );
@@ -98,6 +106,7 @@ describe("SettingsForm", () => {
       });
 
       expect(onModelChangeMock).toHaveBeenCalledWith("model3");
+      expect(onAPIKeyChangeMock).toHaveBeenCalledWith("");
     });
 
     it("should call the onAgentChange handler when the agent changes", () => {
@@ -131,5 +140,16 @@ describe("SettingsForm", () => {
 
       expect(onLanguageChangeMock).toHaveBeenCalledWith("Français");
     });
+
+    it("should call the onAPIKeyChange handler when the API key changes", () => {
+      renderSettingsForm();
+
+      const apiKeyInput = screen.getByTestId("apikey");
+      act(() => {
+        userEvent.type(apiKeyInput, "x");
+      });
+
+      expect(onAPIKeyChangeMock).toHaveBeenCalledWith("sk-...x");
+    });
   });
 });
diff --git a/frontend/src/components/modals/settings/SettingsForm.tsx b/frontend/src/components/modals/settings/SettingsForm.tsx
index caa2d34cd180..21042fbc222d 100644
--- a/frontend/src/components/modals/settings/SettingsForm.tsx
+++ b/frontend/src/components/modals/settings/SettingsForm.tsx
@@ -1,5 +1,7 @@
+import { Input, useDisclosure } from "@nextui-org/react";
 import React, { useEffect } from "react";
 import { useTranslation } from "react-i18next";
+import { FaEye, FaEyeSlash } from "react-icons/fa";
 import { useSelector } from "react-redux";
 import { AvailableLanguages } from "../../../i18n";
 import { I18nKey } from "../../../i18n/declaration";
@@ -14,6 +16,7 @@ interface SettingsFormProps {
   agents: string[];
 
   onModelChange: (model: string) => void;
+  onAPIKeyChange: (apiKey: string) => void;
   onAgentChange: (agent: string) => void;
   onLanguageChange: (language: string) => void;
 }
@@ -23,12 +26,14 @@ function SettingsForm({
   models,
   agents,
   onModelChange,
+  onAPIKeyChange,
   onAgentChange,
   onLanguageChange,
 }: SettingsFormProps) {
   const { t } = useTranslation();
   const { curTaskState } = useSelector((state: RootState) => state.agent);
   const [disabled, setDisabled] = React.useState<boolean>(false);
+  const { isOpen: isVisible, onOpenChange: onVisibleChange } = useDisclosure();
 
   useEffect(() => {
     if (
@@ -47,11 +52,38 @@ function SettingsForm({
         ariaLabel="model"
         items={models.map((model) => ({ value: model, label: model }))}
         defaultKey={settings.LLM_MODEL || models[0]}
-        onChange={onModelChange}
+        onChange={(e) => {
+          onModelChange(e);
+        }}
         tooltip={t(I18nKey.SETTINGS$MODEL_TOOLTIP)}
         allowCustomValue // user can type in a custom LLM model that is not in the list
         disabled={disabled}
       />
+      <Input
+        label="API Key"
+        disabled={disabled}
+        aria-label="apikey"
+        data-testid="apikey"
+        placeholder={t(I18nKey.SETTINGS$API_KEY_PLACEHOLDER)}
+        type={isVisible ? "text" : "password"}
+        value={settings.LLM_API_KEY || ""}
+        onChange={(e) => {
+          onAPIKeyChange(e.target.value);
+        }}
+        endContent={
+          <button
+            className="focus:outline-none"
+            type="button"
+            onClick={onVisibleChange}
+          >
+            {isVisible ? (
+              <FaEye className="text-2xl text-default-400 pointer-events-none" />
+            ) : (
+              <FaEyeSlash className="text-2xl text-default-400 pointer-events-none" />
+            )}
+          </button>
+        }
+      />
       <AutocompleteCombobox
         ariaLabel="agent"
         items={agents.map((agent) => ({ value: agent, label: agent }))}
diff --git a/frontend/src/components/modals/settings/SettingsModal.test.tsx b/frontend/src/components/modals/settings/SettingsModal.test.tsx
index a37502cefe42..8530f185f0eb 100644
--- a/frontend/src/components/modals/settings/SettingsModal.test.tsx
+++ b/frontend/src/components/modals/settings/SettingsModal.test.tsx
@@ -1,14 +1,14 @@
+import { fetchAgents, fetchModels } from "#/api";
+import { initializeAgent } from "#/services/agent";
+import { Settings, getSettings, saveSettings } from "#/services/settings";
+import toast from "#/utils/toast";
 import { act, screen, waitFor } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
+import i18next from "i18next";
 import React from "react";
 import { renderWithProviders } from "test-utils";
 import { Mock } from "vitest";
-import i18next from "i18next";
 import SettingsModal from "./SettingsModal";
-import { Settings, getSettings, saveSettings } from "#/services/settings";
-import { initializeAgent } from "#/services/agent";
-import toast from "#/utils/toast";
-import { fetchAgents, fetchModels } from "#/api";
 
 const toastSpy = vi.spyOn(toast, "settingsChanged");
 const i18nSpy = vi.spyOn(i18next, "changeLanguage");
@@ -98,6 +98,7 @@ describe("SettingsModal", () => {
       LLM_MODEL: "gpt-3.5-turbo",
       AGENT: "MonologueAgent",
       LANGUAGE: "en",
+      LLM_API_KEY: "sk-...",
     };
 
     it("should save the settings", async () => {
diff --git a/frontend/src/components/modals/settings/SettingsModal.tsx b/frontend/src/components/modals/settings/SettingsModal.tsx
index 62456ef217d8..6cee35a1c09c 100644
--- a/frontend/src/components/modals/settings/SettingsModal.tsx
+++ b/frontend/src/components/modals/settings/SettingsModal.tsx
@@ -1,20 +1,20 @@
-import { Spinner } from "@nextui-org/react";
-import React from "react";
-import { useTranslation } from "react-i18next";
-import i18next from "i18next";
+import { fetchAgents, fetchModels } from "#/api";
 import { AvailableLanguages } from "#/i18n";
 import { I18nKey } from "#/i18n/declaration";
-import { fetchAgents, fetchModels } from "#/api";
-import BaseModal from "../base-modal/BaseModal";
-import SettingsForm from "./SettingsForm";
+import { initializeAgent } from "#/services/agent";
 import {
   Settings,
-  saveSettings,
   getSettings,
   getSettingsDifference,
+  saveSettings,
 } from "#/services/settings";
 import toast from "#/utils/toast";
-import { initializeAgent } from "#/services/agent";
+import { Spinner } from "@nextui-org/react";
+import i18next from "i18next";
+import React from "react";
+import { useTranslation } from "react-i18next";
+import BaseModal from "../base-modal/BaseModal";
+import SettingsForm from "./SettingsForm";
 
 interface SettingsProps {
   isOpen: boolean;
@@ -46,6 +46,11 @@ function SettingsModal({ isOpen, onOpenChange }: SettingsProps) {
 
   const handleModelChange = (model: string) => {
     setSettings((prev) => ({ ...prev, LLM_MODEL: model }));
+    // Needs to also reset the API key.
+    const key = localStorage.getItem(
+      `API_KEY_${settings.LLM_MODEL || models[0]}`,
+    );
+    setSettings((prev) => ({ ...prev, LLM_API_KEY: key || "" }));
   };
 
   const handleAgentChange = (agent: string) => {
@@ -60,6 +65,10 @@ function SettingsModal({ isOpen, onOpenChange }: SettingsProps) {
     if (key) setSettings((prev) => ({ ...prev, LANGUAGE: key }));
   };
 
+  const handleAPIKeyChange = (key: string) => {
+    setSettings((prev) => ({ ...prev, LLM_API_KEY: key }));
+  };
+
   const handleSaveSettings = () => {
     const updatedSettings = getSettingsDifference(settings);
     saveSettings(settings);
@@ -69,6 +78,11 @@ function SettingsModal({ isOpen, onOpenChange }: SettingsProps) {
     Object.entries(updatedSettings).forEach(([key, value]) => {
       toast.settingsChanged(`${key} set to "${value}"`);
     });
+
+    localStorage.setItem(
+      `API_KEY_${settings.LLM_MODEL || models[0]}`,
+      settings.LLM_API_KEY,
+    );
   };
 
   return (
@@ -106,6 +120,7 @@ function SettingsModal({ isOpen, onOpenChange }: SettingsProps) {
           onModelChange={handleModelChange}
           onAgentChange={handleAgentChange}
           onLanguageChange={handleLanguageChange}
+          onAPIKeyChange={handleAPIKeyChange}
         />
       )}
     </BaseModal>
diff --git a/frontend/src/i18n/translation.json b/frontend/src/i18n/translation.json
index 6967d26aa229..953c14d8cff4 100644
--- a/frontend/src/i18n/translation.json
+++ b/frontend/src/i18n/translation.json
@@ -324,5 +324,9 @@
   "SETTINGS$DISABLED_RUNNING": {
     "en": "Cannot be changed while the agent is running.",
     "de": "Kann nicht geändert werden während ein Task ausgeführt wird."
+  },
+  "SETTINGS$API_KEY_PLACEHOLDER": {
+    "en": "Enter your API key.",
+    "de": "Model API key."
   }
 }
diff --git a/frontend/src/services/agent.test.ts b/frontend/src/services/agent.test.ts
index 39afa86fef2b..3e86dff12bf3 100644
--- a/frontend/src/services/agent.test.ts
+++ b/frontend/src/services/agent.test.ts
@@ -1,11 +1,11 @@
-import { describe, it, expect, vi } from "vitest";
+import { describe, expect, it, vi } from "vitest";
 
+import { setInitialized } from "#/state/taskSlice";
+import store from "#/store";
 import ActionType from "#/types/ActionType";
-import { Settings } from "./settings";
 import { initializeAgent } from "./agent";
+import { Settings } from "./settings";
 import Socket from "./socket";
-import store from "#/store";
-import { setInitialized } from "#/state/taskSlice";
 
 const sendSpy = vi.spyOn(Socket, "send");
 const dispatchSpy = vi.spyOn(store, "dispatch");
@@ -16,6 +16,7 @@ describe("initializeAgent", () => {
       LLM_MODEL: "llm_value",
       AGENT: "agent_value",
       LANGUAGE: "language_value",
+      LLM_API_KEY: "sk-...",
     };
 
     const event = {
diff --git a/frontend/src/services/settings.ts b/frontend/src/services/settings.ts
index 651e2defde96..29129b13f371 100644
--- a/frontend/src/services/settings.ts
+++ b/frontend/src/services/settings.ts
@@ -2,12 +2,14 @@ export type Settings = {
   LLM_MODEL: string;
   AGENT: string;
   LANGUAGE: string;
+  LLM_API_KEY: string;
 };
 
 export const DEFAULT_SETTINGS: Settings = {
   LLM_MODEL: "gpt-3.5-turbo",
   AGENT: "MonologueAgent",
   LANGUAGE: "en",
+  LLM_API_KEY: "",
 };
 
 const validKeys = Object.keys(DEFAULT_SETTINGS) as (keyof Settings)[];
@@ -19,6 +21,10 @@ export const getSettings = (): Settings => ({
   LLM_MODEL: localStorage.getItem("LLM_MODEL") || DEFAULT_SETTINGS.LLM_MODEL,
   AGENT: localStorage.getItem("AGENT") || DEFAULT_SETTINGS.AGENT,
   LANGUAGE: localStorage.getItem("LANGUAGE") || DEFAULT_SETTINGS.LANGUAGE,
+  LLM_API_KEY:
+    localStorage.getItem(
+      `API_KEY_${localStorage.getItem("LLM_MODEL") || DEFAULT_SETTINGS.LLM_MODEL}`,
+    ) || DEFAULT_SETTINGS.LLM_API_KEY,
 });
 
 /**
diff --git a/frontend/src/types/ConfigType.tsx b/frontend/src/types/ConfigType.tsx
index d72bd7cf5592..526558a028cd 100644
--- a/frontend/src/types/ConfigType.tsx
+++ b/frontend/src/types/ConfigType.tsx
@@ -2,12 +2,14 @@ enum ArgConfigType {
   LLM_MODEL = "LLM_MODEL",
   AGENT = "AGENT",
   LANGUAGE = "LANGUAGE",
+  LLM_API_KEY = "LLM_API_KEY",
 }
 
 const SupportedSettings: string[] = [
   ArgConfigType.LLM_MODEL,
   ArgConfigType.AGENT,
   ArgConfigType.LANGUAGE,
+  ArgConfigType.LLM_API_KEY,
 ];
 
 export { ArgConfigType, SupportedSettings };
diff --git a/opendevin/server/agent/agent.py b/opendevin/server/agent/agent.py
index 66c8b39be17d..7e39b87fc428 100644
--- a/opendevin/server/agent/agent.py
+++ b/opendevin/server/agent/agent.py
@@ -1,5 +1,5 @@
 import asyncio
-from typing import Optional, Dict, List
+from typing import Dict, List, Optional
 
 from opendevin import config
 from opendevin.action import (
@@ -136,7 +136,7 @@ async def create_controller(self, start_event: dict):
         }  # remove empty values, prevent FE from sending empty strings
         agent_cls = self.get_arg_or_default(args, ConfigType.AGENT)
         model = self.get_arg_or_default(args, ConfigType.LLM_MODEL)
-        api_key = config.get(ConfigType.LLM_API_KEY)
+        api_key = self.get_arg_or_default(args, ConfigType.LLM_API_KEY)
         api_base = config.get(ConfigType.LLM_BASE_URL)
         max_iterations = self.get_arg_or_default(args, ConfigType.MAX_ITERATIONS)
         max_chars = self.get_arg_or_default(args, ConfigType.MAX_CHARS)


Code review comments:
diff_hunk: @@ -46,6 +46,11 @@ function SettingsModal({ isOpen, onOpenChange }: SettingsProps) {
 
   const handleModelChange = (model: string) => {
     setSettings((prev) => ({ ...prev, LLM_MODEL: model }));
+    // Needs to also reset the API key.
+    const key = localStorage.getItem(
+      `API_KEY_${settings.LLM_MODEL || models[0]}`,
+    );
+    setSettings((prev) => ({ ...prev, LLM_API_KEY: key || "" }));
path: frontend/src/components/modals/settings/SettingsModal.tsx
text: @user1:
You could merge the two calls to `setSettings` into one.