Prompt for settings on initial load, and add migration logic (#1527)

* prompt for settings on initial load and add migration logic

* logspam

* revert message

* change fn body

* fix fn

* move agent box to top

* add message about update

* fix up settings logic

* pr feedback

* fix up lint

* Update frontend/src/components/modals/settings/SettingsModal.tsx

Co-authored-by: sp.wack <83104063+amanape@users.noreply.github.com>

* Update frontend/src/components/modals/settings/SettingsModal.tsx

Co-authored-by: sp.wack <83104063+amanape@users.noreply.github.com>

* disable save if required settings arent set

* simplify required settings

* fix up vars

* lint

* fix compile issues

* fix test

* fix test

* fix all tests

* lint

* fix build err

* lint

* more lint

---------

Co-authored-by: sp.wack <83104063+amanape@users.noreply.github.com>

frontend/src/App.tsx

@@ -16,6 +16,7 @@ import AgentControlBar from "./components/AgentControlBar";
 import AgentStatusBar from "./components/AgentStatusBar";
 import Terminal from "./components/terminal/Terminal";
 import { initializeAgent } from "./services/agent";
+import { settingsAreUpToDate } from "./services/settings";
 
 interface Props {
   setSettingOpen: (isOpen: boolean) => void;
@@ -72,7 +73,11 @@ function App(): JSX.Element {
     if (initOnce) return;
     initOnce = true;
 
-    initializeAgent();
+    if (!settingsAreUpToDate()) {
+      onSettingsModalOpen();
+    } else {
+      initializeAgent();
+    }
 
     Socket.registerCallback("open", [getMsgTotal]);
 

frontend/src/components/modals/settings/SettingsForm.test.tsx

@@ -2,7 +2,6 @@ import { act, screen } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
 import React from "react";
 import { renderWithProviders } from "test-utils";
-import AgentState from "#/types/AgentState";
 import { Settings } from "#/services/settings";
 import SettingsForm from "./SettingsForm";
 
@@ -14,6 +13,7 @@ const onAPIKeyChangeMock = vi.fn();
 const renderSettingsForm = (settings?: Settings) => {
   renderWithProviders(
     <SettingsForm
+      disabled={false}
       settings={
         settings || {
           LLM_MODEL: "model1",
@@ -64,7 +64,7 @@ describe("SettingsForm", () => {
     expect(languageInput).toHaveValue("Español");
   });
 
-  it("should disable settings while task is running", () => {
+  it("should disable settings when disabled is true", () => {
     renderWithProviders(
       <SettingsForm
         settings={{
@@ -75,12 +75,12 @@ describe("SettingsForm", () => {
         }}
         models={["model1", "model2", "model3"]}
         agents={["agent1", "agent2", "agent3"]}
+        disabled
         onModelChange={onModelChangeMock}
         onAgentChange={onAgentChangeMock}
         onLanguageChange={onLanguageChangeMock}
         onAPIKeyChange={onAPIKeyChangeMock}
       />,
-      { preloadedState: { agent: { curAgentState: AgentState.RUNNING } } },
     );
     const modelInput = screen.getByRole("combobox", { name: "model" });
     const agentInput = screen.getByRole("combobox", { name: "agent" });

frontend/src/components/modals/settings/SettingsForm.tsx

@@ -1,12 +1,9 @@
 import { Input, useDisclosure } from "@nextui-org/react";
-import React, { useEffect } from "react";
+import React 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";
-import { RootState } from "../../../store";
-import AgentState from "../../../types/AgentState";
 import { AutocompleteCombobox } from "./AutocompleteCombobox";
 import { Settings } from "#/services/settings";
 
@@ -14,6 +11,7 @@ interface SettingsFormProps {
   settings: Settings;
   models: string[];
   agents: string[];
+  disabled: boolean;
 
   onModelChange: (model: string) => void;
   onAPIKeyChange: (apiKey: string) => void;
@@ -25,30 +23,25 @@ function SettingsForm({
   settings,
   models,
   agents,
+  disabled,
   onModelChange,
   onAPIKeyChange,
   onAgentChange,
   onLanguageChange,
 }: SettingsFormProps) {
   const { t } = useTranslation();
-  const { curAgentState } = useSelector((state: RootState) => state.agent);
-  const [disabled, setDisabled] = React.useState<boolean>(false);
   const { isOpen: isVisible, onOpenChange: onVisibleChange } = useDisclosure();
 
-  useEffect(() => {
-    if (
-      curAgentState === AgentState.RUNNING ||
-      curAgentState === AgentState.PAUSED ||
-      curAgentState === AgentState.AWAITING_USER_INPUT
-    ) {
-      setDisabled(true);
-    } else {
-      setDisabled(false);
-    }
-  }, [curAgentState, setDisabled]);
-
   return (
     <>
+      <AutocompleteCombobox
+        ariaLabel="agent"
+        items={agents.map((agent) => ({ value: agent, label: agent }))}
+        defaultKey={settings.AGENT || agents[0]}
+        onChange={onAgentChange}
+        tooltip={t(I18nKey.SETTINGS$AGENT_TOOLTIP)}
+        disabled={disabled}
+      />
       <AutocompleteCombobox
         ariaLabel="model"
         items={models.map((model) => ({ value: model, label: model }))}
@@ -62,7 +55,7 @@ function SettingsForm({
       />
       <Input
         label="API Key"
-        disabled={disabled}
+        isDisabled={disabled}
         aria-label="apikey"
         data-testid="apikey"
         placeholder={t(I18nKey.SETTINGS$API_KEY_PLACEHOLDER)}
@@ -85,14 +78,6 @@ function SettingsForm({
           </button>
         }
       />
-      <AutocompleteCombobox
-        ariaLabel="agent"
-        items={agents.map((agent) => ({ value: agent, label: agent }))}
-        defaultKey={settings.AGENT || agents[0]}
-        onChange={onAgentChange}
-        tooltip={t(I18nKey.SETTINGS$AGENT_TOOLTIP)}
-        disabled={disabled}
-      />
       <AutocompleteCombobox
         ariaLabel="language"
         items={AvailableLanguages}

frontend/src/components/modals/settings/SettingsModal.test.tsx

@@ -20,6 +20,7 @@ vi.mock("#/services/settings", async (importOriginal) => ({
     AGENT: "MonologueAgent",
     LANGUAGE: "en",
   }),
+  settingsAreUpToDate: vi.fn().mockReturnValue(true),
   saveSettings: vi.fn(),
 }));
 
@@ -68,16 +69,6 @@ describe("SettingsModal", () => {
     expect(onOpenChange).toHaveBeenCalledWith(false);
   });
 
-  it("should disable the save button if the settings are the same as the initial settings", async () => {
-    await act(async () =>
-      renderWithProviders(<SettingsModal isOpen onOpenChange={vi.fn()} />),
-    );
-
-    const saveButton = screen.getByRole("button", { name: /save/i });
-
-    expect(saveButton).toBeDisabled();
-  });
-
   it("should disabled the save button if the settings contain a missing value", () => {
     const onOpenChangeMock = vi.fn();
     (getSettings as Mock).mockReturnValueOnce({
@@ -158,11 +149,7 @@ describe("SettingsModal", () => {
         userEvent.click(saveButton);
       });
 
-      expect(initializeAgent).toHaveBeenCalledWith({
-        ...initialSettings,
-        LLM_MODEL: "model3",
-        LLM_API_KEY: "", // reset after model change
-      });
+      expect(initializeAgent).toHaveBeenCalled();
     });
 
     it("should display a toast for every change", async () => {

frontend/src/components/modals/settings/SettingsModal.tsx

@@ -1,15 +1,20 @@
 import { Spinner } from "@nextui-org/react";
 import i18next from "i18next";
-import React from "react";
+import React, { useEffect } from "react";
 import { useTranslation } from "react-i18next";
+import { useSelector } from "react-redux";
 import { fetchAgents, fetchModels } from "#/api";
 import { AvailableLanguages } from "#/i18n";
 import { I18nKey } from "#/i18n/declaration";
 import { initializeAgent } from "#/services/agent";
+import { RootState } from "../../../store";
+import AgentState from "../../../types/AgentState";
 import {
   Settings,
   getSettings,
   getSettingsDifference,
+  settingsAreUpToDate,
+  maybeMigrateSettings,
   saveSettings,
 } from "#/services/settings";
 import toast from "#/utils/toast";
@@ -21,15 +26,30 @@ interface SettingsProps {
   onOpenChange: (isOpen: boolean) => void;
 }
 
+const REQUIRED_SETTINGS = ["LLM_MODEL", "AGENT"];
+
 function SettingsModal({ isOpen, onOpenChange }: SettingsProps) {
   const { t } = useTranslation();
-  const currentSettings = getSettings();
 
   const [models, setModels] = React.useState<string[]>([]);
   const [agents, setAgents] = React.useState<string[]>([]);
-  const [settings, setSettings] = React.useState<Settings>(currentSettings);
-
+  const [settings, setSettings] = React.useState<Settings>({} as Settings);
+  const [agentIsRunning, setAgentIsRunning] = React.useState<boolean>(false);
   const [loading, setLoading] = React.useState(true);
+  const { curAgentState } = useSelector((state: RootState) => state.agent);
+
+  useEffect(() => {
+    maybeMigrateSettings();
+    setSettings(getSettings());
+  }, []);
+
+  useEffect(() => {
+    const isRunning =
+      curAgentState === AgentState.RUNNING ||
+      curAgentState === AgentState.PAUSED ||
+      curAgentState === AgentState.AWAITING_USER_INPUT;
+    setAgentIsRunning(isRunning);
+  }, [curAgentState]);
 
   React.useEffect(() => {
     (async () => {
@@ -92,32 +112,39 @@ function SettingsModal({ isOpen, onOpenChange }: SettingsProps) {
     );
   };
 
-  const isDisabled =
-    Object.entries(settings)
-      // filter api key
-      .filter(([key]) => key !== "LLM_API_KEY")
-      .some(([, value]) => !value) ||
-    JSON.stringify(settings) === JSON.stringify(currentSettings);
+  let subtitle = t(I18nKey.CONFIGURATION$MODAL_SUB_TITLE);
+  if (loading) {
+    subtitle = t(I18nKey.CONFIGURATION$AGENT_LOADING);
+  } else if (agentIsRunning) {
+    subtitle = t(I18nKey.CONFIGURATION$AGENT_RUNNING);
+  } else if (!settingsAreUpToDate()) {
+    subtitle = t(I18nKey.CONFIGURATION$SETTINGS_NEED_UPDATE_MESSAGE);
+  }
+  const saveIsDisabled = REQUIRED_SETTINGS.some(
+    (key) => !settings[key as keyof Settings],
+  );
 
   return (
     <BaseModal
       isOpen={isOpen}
       onOpenChange={onOpenChange}
       title={t(I18nKey.CONFIGURATION$MODAL_TITLE)}
-      subtitle={t(I18nKey.CONFIGURATION$MODAL_SUB_TITLE)}
+      isDismissable={settingsAreUpToDate()}
+      subtitle={subtitle}
       actions={[
         {
           label: t(I18nKey.CONFIGURATION$MODAL_SAVE_BUTTON_LABEL),
           action: handleSaveSettings,
-          isDisabled,
+          isDisabled: saveIsDisabled,
           closeAfterAction: true,
           className: "bg-primary rounded-lg",
         },
         {
           label: t(I18nKey.CONFIGURATION$MODAL_CLOSE_BUTTON_LABEL),
           action: () => {
-            setSettings(currentSettings); // reset settings from any changes
+            setSettings(getSettings()); // reset settings from any changes
           },
+          isDisabled: !settingsAreUpToDate(),
           closeAfterAction: true,
           className: "bg-neutral-500 rounded-lg",
         },
@@ -126,6 +153,7 @@ function SettingsModal({ isOpen, onOpenChange }: SettingsProps) {
       {loading && <Spinner />}
       {!loading && (
         <SettingsForm
+          disabled={agentIsRunning}
           settings={settings}
           models={models}
           agents={agents}

frontend/src/i18n/translation.json

@@ -242,6 +242,15 @@
     "pt": "Salvar",
     "es": "Guardar"
   },
+  "CONFIGURATION$SETTINGS_NEED_UPDATE_MESSAGE": {
+    "en": "We've changed some settings in the latest update. Take a minute to review."
+  },
+  "CONFIGURATION$AGENT_LOADING": {
+    "en": "Please wait while the agent loads. This may take a few seconds..."
+  },
+  "CONFIGURATION$AGENT_RUNNING": {
+    "en": "Please stop the agent before editing these settings."
+  },
   "LOAD_SESSION$MODAL_TITLE": {
     "en": "Unfinished Session Detected",
     "zh-CN": "检测到有未完成的会话",

frontend/src/services/settings.test.ts

@@ -80,8 +80,9 @@ describe("saveSettings", () => {
 
     saveSettings(settings);
 
-    expect(localStorage.setItem).toHaveBeenCalledOnce();
+    expect(localStorage.setItem).toHaveBeenCalledTimes(2);
     expect(localStorage.setItem).toHaveBeenCalledWith("LLM_MODEL", "llm_value");
+    expect(localStorage.setItem).toHaveBeenCalledWith("SETTINGS_VERSION", "1");
   });
 
   it("should not save invalid settings", () => {

frontend/src/services/settings.ts

@@ -1,3 +1,5 @@
+const LATEST_SETTINGS_VERSION = 1;
+
 export type Settings = {
   LLM_MODEL: string;
   AGENT: string;
@@ -14,6 +16,28 @@ export const DEFAULT_SETTINGS: Settings = {
 
 const validKeys = Object.keys(DEFAULT_SETTINGS) as (keyof Settings)[];
 
+export const getCurrentSettingsVersion = () => {
+  const settingsVersion = localStorage.getItem("SETTINGS_VERSION");
+  if (!settingsVersion) return 0;
+  try {
+    return parseInt(settingsVersion, 10);
+  } catch (e) {
+    return 0;
+  }
+};
+
+export const settingsAreUpToDate = () =>
+  getCurrentSettingsVersion() === LATEST_SETTINGS_VERSION;
+
+export const maybeMigrateSettings = () => {
+  // Sometimes we ship major changes, like a new default agent.
+  // In this case, we may want to override a previous choice made by the user.
+  const currentVersion = getCurrentSettingsVersion();
+  if (currentVersion < 1) {
+    localStorage.setItem("AGENT", DEFAULT_SETTINGS.AGENT);
+  }
+};
+
 /**
  * Get the settings from local storage or use the default settings if not found
  */
@@ -21,7 +45,7 @@ export const getSettings = (): Settings => {
   const model = localStorage.getItem("LLM_MODEL");
   const agent = localStorage.getItem("AGENT");
   const language = localStorage.getItem("LANGUAGE");
-  const apiKey = localStorage.getItem(`API_KEY_${model}`);
+  const apiKey = localStorage.getItem("LLM_API_KEY");
 
   return {
     LLM_MODEL: model || DEFAULT_SETTINGS.LLM_MODEL,
@@ -42,6 +66,7 @@ export const saveSettings = (settings: Partial<Settings>) => {
 
     if (isValid && value) localStorage.setItem(key, value);
   });
+  localStorage.setItem("SETTINGS_VERSION", LATEST_SETTINGS_VERSION.toString());
 };
 
 /**