fix(frontend): Hide modal when in settings page if first time (#6792)

All-Hands-AI · Feb 18, 2025 · e3e00ed · e3e00ed
1 parent 96d1992
commit e3e00ed
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 16 deletions.
diff --git a/frontend/__tests__/routes/home.test.tsx b/frontend/__tests__/routes/home.test.tsx
@@ -39,12 +39,12 @@ describe("Home Screen", () => {
           Component: Home,
           path: "/",
         },
+        {
+          Component: SettingsScreen,
+          path: "/settings",
+        },
       ],
     },
-    {
-      Component: SettingsScreen,
-      path: "/settings",
-    },
   ]);
 
   afterEach(() => {
@@ -96,6 +96,9 @@ describe("Home Screen", () => {
       const user = userEvent.setup();
       renderWithProviders(<RouterStub initialEntries={["/"]} />);
 
+      const settingsScreen = screen.queryByTestId("settings-screen");
+      expect(settingsScreen).not.toBeInTheDocument();
+
       const settingsModal = await screen.findByTestId("ai-config-modal");
       expect(settingsModal).toBeInTheDocument();
 
@@ -104,11 +107,11 @@ describe("Home Screen", () => {
       );
       await user.click(advancedSettingsButton);
 
+      const settingsScreenAfter = await screen.findByTestId("settings-screen");
+      expect(settingsScreenAfter).toBeInTheDocument();
+
       const settingsModalAfter = screen.queryByTestId("ai-config-modal");
       expect(settingsModalAfter).not.toBeInTheDocument();
-
-      const settingsScreen = await screen.findByTestId("settings-screen");
-      expect(settingsScreen).toBeInTheDocument();
     });
   });
 });
diff --git a/frontend/__tests__/routes/settings.test.tsx b/frontend/__tests__/routes/settings.test.tsx
@@ -696,7 +696,7 @@ describe("Settings Screen", () => {
 
       expect(saveSettingsSpy).toHaveBeenCalledWith(
         expect.objectContaining({
-          llm_api_key: undefined,
+          llm_api_key: "", // empty because it's not set previously
           github_token: undefined,
           language: "no",
         }),
@@ -735,7 +735,7 @@ describe("Settings Screen", () => {
       expect(saveSettingsSpy).toHaveBeenCalledWith(
         expect.objectContaining({
           github_token: undefined,
-          llm_api_key: undefined,
+          llm_api_key: "", // empty because it's not set previously
           llm_model: "openai/gpt-4o",
         }),
       );
@@ -900,5 +900,55 @@ describe("Settings Screen", () => {
         }),
       );
     });
+
+    it("should send an empty LLM API Key if the user submits an empty string", async () => {
+      const user = userEvent.setup();
+      renderSettingsScreen();
+
+      const input = await screen.findByTestId("llm-api-key-input");
+      expect(input).toHaveValue("");
+
+      const saveButton = screen.getByText("Save Changes");
+      await user.click(saveButton);
+
+      expect(saveSettingsSpy).toHaveBeenCalledWith(
+        expect.objectContaining({ llm_api_key: "" }),
+      );
+    });
+
+    it("should not send an empty LLM API Key if the user submits an empty string but already has it set", async () => {
+      const user = userEvent.setup();
+      getSettingsSpy.mockResolvedValue({
+        ...MOCK_DEFAULT_USER_SETTINGS,
+        llm_api_key: "**********",
+      });
+
+      renderSettingsScreen();
+
+      const input = await screen.findByTestId("llm-api-key-input");
+      expect(input).toHaveValue("");
+
+      const saveButton = screen.getByText("Save Changes");
+      await user.click(saveButton);
+
+      expect(saveSettingsSpy).toHaveBeenCalledWith(
+        expect.objectContaining({ llm_api_key: undefined }),
+      );
+    });
+
+    it("should submit the LLM API Key if it is the first time the user sets it", async () => {
+      const user = userEvent.setup();
+      renderSettingsScreen();
+
+      const input = await screen.findByTestId("llm-api-key-input");
+      await user.type(input, "new-api-key");
+
+      const saveButton = screen.getByText("Save Changes");
+      await user.click(saveButton);
+
+      expect(saveSettingsSpy).toHaveBeenCalledWith(
+        expect.objectContaining({ llm_api_key: "new-api-key" }),
+      );
+    });
   });
 });
diff --git a/frontend/src/components/features/sidebar/sidebar.tsx b/frontend/src/components/features/sidebar/sidebar.tsx
@@ -3,7 +3,7 @@ import { FaListUl } from "react-icons/fa";
 import { useDispatch } from "react-redux";
 import posthog from "posthog-js";
 import toast from "react-hot-toast";
-import { NavLink } from "react-router";
+import { NavLink, useLocation } from "react-router";
 import { useGitHubUser } from "#/hooks/query/use-github-user";
 import { UserActions } from "./user-actions";
 import { AllHandsLogoButton } from "#/components/shared/buttons/all-hands-logo-button";
@@ -25,6 +25,7 @@ import { useConfig } from "#/hooks/query/use-config";
 import { cn } from "#/utils/utils";
 
 export function Sidebar() {
+  const location = useLocation();
   const dispatch = useDispatch();
   const endSession = useEndSession();
   const user = useGitHubUser();
@@ -43,20 +44,27 @@ export function Sidebar() {
     React.useState(false);
 
   React.useEffect(() => {
-    // We don't show toast errors for settings in the global error handler
-    // because we have a special case for 404 errors
-    if (
+    if (location.pathname === "/settings") {
+      setSettingsModalIsOpen(false);
+    } else if (
       !isFetchingSettings &&
       settingsIsError &&
       settingsError?.status !== 404
     ) {
+      // We don't show toast errors for settings in the global error handler
+      // because we have a special case for 404 errors
       toast.error(
         "Something went wrong while fetching settings. Please reload the page.",
       );
     } else if (settingsError?.status === 404) {
       setSettingsModalIsOpen(true);
     }
-  }, [settingsError?.status, settingsError, isFetchingSettings]);
+  }, [
+    settingsError?.status,
+    settingsError,
+    isFetchingSettings,
+    location.pathname,
+  ]);
 
   const handleEndSession = () => {
     dispatch(setCurrentAgentState(AgentState.LOADING));

diff --git a/frontend/src/components/shared/modals/settings/settings-modal.tsx b/frontend/src/components/shared/modals/settings/settings-modal.tsx
@@ -18,7 +18,7 @@ export function SettingsModal({ onClose, settings }: SettingsModalProps) {
   const { t } = useTranslation();
 
   return (
-    <ModalBackdrop onClose={onClose}>
+    <ModalBackdrop>
       <div
         data-testid="ai-config-modal"
         className="bg-root-primary min-w-[384px] p-6 rounded-xl flex flex-col gap-2"

diff --git a/frontend/src/routes/settings.tsx b/frontend/src/routes/settings.tsx
@@ -131,7 +131,11 @@ function SettingsScreen() {
         user_consents_to_analytics: userConsentsToAnalytics,
         LLM_MODEL: customLlmModel || fullLlmModel,
         LLM_BASE_URL: formData.get("base-url-input")?.toString() || "",
-        LLM_API_KEY: formData.get("llm-api-key-input")?.toString() || undefined,
+        LLM_API_KEY:
+          formData.get("llm-api-key-input")?.toString() ||
+          (isLLMKeySet
+            ? undefined // don't update if it's already set
+            : ""), // reset if it's first time save to avoid 500 error
         AGENT: formData.get("agent-input")?.toString(),
         SECURITY_ANALYZER:
           formData.get("security-analyzer-input")?.toString() || "",