fix(frontend): Wait for fetched settings instead of loading default ones (#6193)

frontend/__tests__/components/features/sidebar/sidebar.test.tsx

@@ -1,10 +1,12 @@
-import { screen } from "@testing-library/react";
+import { screen, within } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
-import { describe, expect, it } from "vitest";
+import { afterEach, describe, expect, it, vi } from "vitest";
 import { renderWithProviders } from "test-utils";
 import { createRoutesStub } from "react-router";
 import { Sidebar } from "#/components/features/sidebar/sidebar";
 import { MULTI_CONVERSATION_UI } from "#/utils/feature-flags";
+import OpenHands from "#/api/open-hands";
+import { MOCK_USER_PREFERENCES } from "#/mocks/handlers";
 
 const renderSidebar = () => {
   const RouterStub = createRoutesStub([
@@ -43,4 +45,101 @@ describe("Sidebar", () => {
       ).not.toBeInTheDocument();
     },
   );
+
+  describe("Settings", () => {
+    const getSettingsSpy = vi.spyOn(OpenHands, "getSettings");
+    const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings");
+
+    afterEach(() => {
+      vi.clearAllMocks();
+    });
+
+    it("should fetch settings data on mount", () => {
+      renderSidebar();
+      expect(getSettingsSpy).toHaveBeenCalledOnce();
+    });
+
+    it("should send all settings data when saving AI configuration", async () => {
+      const user = userEvent.setup();
+      renderSidebar();
+
+      const settingsButton = screen.getByTestId("settings-button");
+      await user.click(settingsButton);
+
+      const settingsModal = screen.getByTestId("ai-config-modal");
+      const saveButton = within(settingsModal).getByTestId(
+        "save-settings-button",
+      );
+      await user.click(saveButton);
+
+      expect(saveSettingsSpy).toHaveBeenCalledWith({
+        ...MOCK_USER_PREFERENCES.settings,
+        // the actual values are falsey (null or "") but we're checking for undefined
+        llm_api_key: undefined,
+        llm_base_url: undefined,
+        security_analyzer: undefined,
+      });
+    });
+
+    it("should send all settings data when saving account settings", async () => {
+      const user = userEvent.setup();
+      renderSidebar();
+
+      const userAvatar = screen.getByTestId("user-avatar");
+      await user.click(userAvatar);
+
+      const menu = screen.getByTestId("account-settings-context-menu");
+      const accountSettingsButton = within(menu).getByTestId(
+        "account-settings-button",
+      );
+      await user.click(accountSettingsButton);
+
+      const accountSettingsModal = screen.getByTestId("account-settings-form");
+      const saveButton =
+        within(accountSettingsModal).getByTestId("save-settings");
+      await user.click(saveButton);
+
+      expect(saveSettingsSpy).toHaveBeenCalledWith({
+        ...MOCK_USER_PREFERENCES.settings,
+        llm_api_key: undefined, // null or undefined
+      });
+    });
+
+    it("should not reset AI configuration when saving account settings", async () => {
+      const user = userEvent.setup();
+      renderSidebar();
+
+      const userAvatar = screen.getByTestId("user-avatar");
+      await user.click(userAvatar);
+
+      const menu = screen.getByTestId("account-settings-context-menu");
+      const accountSettingsButton = within(menu).getByTestId(
+        "account-settings-button",
+      );
+      await user.click(accountSettingsButton);
+
+      const accountSettingsModal = screen.getByTestId("account-settings-form");
+
+      const languageInput =
+        within(accountSettingsModal).getByLabelText(/language/i);
+      await user.click(languageInput);
+
+      const norskOption = screen.getByText(/norsk/i);
+      await user.click(norskOption);
+
+      const tokenInput =
+        within(accountSettingsModal).getByLabelText(/github token/i);
+      await user.type(tokenInput, "new-token");
+
+      const saveButton =
+        within(accountSettingsModal).getByTestId("save-settings");
+      await user.click(saveButton);
+
+      expect(saveSettingsSpy).toHaveBeenCalledWith({
+        ...MOCK_USER_PREFERENCES.settings,
+        language: "no",
+        llm_api_key: undefined, // null or undefined
+      });
+    });
+  });
 });

frontend/src/components/features/context-menu/account-settings-context-menu.tsx

@@ -27,7 +27,10 @@ export function AccountSettingsContextMenu({
       ref={ref}
       className="absolute left-full -top-1 z-10"
     >
-      <ContextMenuListItem onClick={onClickAccountSettings}>
+      <ContextMenuListItem
+        testId="account-settings-button"
+        onClick={onClickAccountSettings}
+      >
         {t(I18nKey.ACCOUNT_SETTINGS$SETTINGS)}
       </ContextMenuListItem>
       <ContextMenuSeparator />

frontend/src/components/features/sidebar/sidebar.tsx

@@ -12,7 +12,7 @@ import { SettingsButton } from "#/components/shared/buttons/settings-button";
 import { LoadingSpinner } from "#/components/shared/loading-spinner";
 import { AccountSettingsModal } from "#/components/shared/modals/account-settings/account-settings-modal";
 import { SettingsModal } from "#/components/shared/modals/settings/settings-modal";
-import { useSettingsUpToDate } from "#/context/settings-up-to-date-context";
+import { useCurrentSettings } from "#/context/settings-context";
 import { useSettings } from "#/hooks/query/use-settings";
 import { ConversationPanel } from "../conversation-panel/conversation-panel";
 import { MULTI_CONVERSATION_UI } from "#/utils/feature-flags";
@@ -28,8 +28,13 @@ export function Sidebar() {
   const user = useGitHubUser();
   const { data: isAuthed } = useIsAuthed();
   const { logout } = useAuth();
-  const { data: settings, isError: settingsIsError } = useSettings();
-  const { isUpToDate: settingsAreUpToDate } = useSettingsUpToDate();
+  const {
+    data: settings,
+    isError: settingsIsError,
+    isSuccess: settingsSuccessfulyFetched,
+  } = useSettings();
+
+  const { isUpToDate: settingsAreUpToDate } = useCurrentSettings();
 
   const [accountSettingsModalOpen, setAccountSettingsModalOpen] =
     React.useState(false);
@@ -106,7 +111,7 @@ export function Sidebar() {
         <AccountSettingsModal onClose={handleAccountSettingsModalClose} />
       )}
       {settingsIsError ||
-        (showSettingsModal && (
+        (showSettingsModal && settingsSuccessfulyFetched && (
           <SettingsModal
             settings={settings}
             onClose={() => setSettingsModalIsOpen(false)}

frontend/src/components/shared/modals/account-settings/account-settings-form.tsx

@@ -13,7 +13,7 @@ import { ModalButton } from "../../buttons/modal-button";
 import { CustomInput } from "../../custom-input";
 import { FormFieldset } from "../../form-fieldset";
 import { useConfig } from "#/hooks/query/use-config";
-import { useSaveSettings } from "#/hooks/mutation/use-save-settings";
+import { useCurrentSettings } from "#/context/settings-context";
 
 interface AccountSettingsFormProps {
   onClose: () => void;
@@ -30,10 +30,10 @@ export function AccountSettingsForm({
 }: AccountSettingsFormProps) {
   const { gitHubToken, setGitHubToken, logout } = useAuth();
   const { data: config } = useConfig();
-  const { mutate: saveSettings } = useSaveSettings();
+  const { saveUserSettings } = useCurrentSettings();
   const { t } = useTranslation();
 
-  const handleSubmit = (event: React.FormEvent<HTMLFormElement>) => {
+  const handleSubmit = async (event: React.FormEvent<HTMLFormElement>) => {
     event.preventDefault();
     const formData = new FormData(event.currentTarget);
 
@@ -50,7 +50,7 @@ export function AccountSettingsForm({
         ({ label }) => label === language,
       )?.value;
 
-      if (languageKey) saveSettings({ LANGUAGE: languageKey });
+      if (languageKey) await saveUserSettings({ LANGUAGE: languageKey });
     }
 
     handleCaptureConsent(analytics);
@@ -61,7 +61,7 @@ export function AccountSettingsForm({
   };
 
   return (
-    <ModalBody>
+    <ModalBody testID="account-settings-form">
       <form className="flex flex-col w-full gap-6" onSubmit={handleSubmit}>
         <div className="w-full flex flex-col gap-2">
           <BaseModalTitle title="Account Settings" />
@@ -137,6 +137,7 @@ export function AccountSettingsForm({
 
         <div className="flex flex-col gap-2 w-full">
           <ModalButton
+            testId="save-settings"
             type="submit"
             intent="account"
             text={t(I18nKey.ACCOUNT_SETTINGS_MODAL$SAVE)}

frontend/src/components/shared/modals/account-settings/account-settings-modal.tsx

@@ -18,7 +18,7 @@ export function AccountSettingsModal({ onClose }: AccountSettingsModalProps) {
     <ModalBackdrop onClose={onClose}>
       <AccountSettingsForm
         onClose={onClose}
-        selectedLanguage={settings.LANGUAGE}
+        selectedLanguage={settings?.LANGUAGE || "en"}
         gitHubError={user.isError}
         analyticsConsent={analyticsConsent}
       />

frontend/src/components/shared/modals/settings/settings-form.tsx

@@ -19,10 +19,10 @@ import { CustomModelInput } from "../../inputs/custom-model-input";
 import { SecurityAnalyzerInput } from "../../inputs/security-analyzers-input";
 import { ModalBackdrop } from "../modal-backdrop";
 import { ModelSelector } from "./model-selector";
-import { useSaveSettings } from "#/hooks/mutation/use-save-settings";
 
 import { RuntimeSizeSelector } from "./runtime-size-selector";
 import { useConfig } from "#/hooks/query/use-config";
+import { useCurrentSettings } from "#/context/settings-context";
 
 interface SettingsFormProps {
   disabled?: boolean;
@@ -41,7 +41,7 @@ export function SettingsForm({
   securityAnalyzers,
   onClose,
 }: SettingsFormProps) {
-  const { mutateAsync: saveSettings } = useSaveSettings();
+  const { saveUserSettings } = useCurrentSettings();
   const endSession = useEndSession();
   const { data: config } = useConfig();
 
@@ -95,7 +95,8 @@ export function SettingsForm({
     const newSettings = extractSettings(formData);
 
     saveSettingsView(isUsingAdvancedOptions ? "advanced" : "basic");
-    await saveSettings(newSettings, { onSuccess: onClose });
+    await saveUserSettings(newSettings);
+    onClose();
     resetOngoingSession();
 
     posthog.capture("settings_saved", {
@@ -107,7 +108,8 @@ export function SettingsForm({
   };
 
   const handleConfirmResetSettings = async () => {
-    await saveSettings(getDefaultSettings(), { onSuccess: onClose });
+    await saveUserSettings(getDefaultSettings());
+    onClose();
     resetOngoingSession();
     posthog.capture("settings_reset");
   };

frontend/src/context/settings-context.tsx

@@ -0,0 +1,70 @@
+import React from "react";
+import {
+  LATEST_SETTINGS_VERSION,
+  Settings,
+  settingsAreUpToDate,
+} from "#/services/settings";
+import { useSettings } from "#/hooks/query/use-settings";
+import { useSaveSettings } from "#/hooks/mutation/use-save-settings";
+
+interface SettingsContextType {
+  isUpToDate: boolean;
+  setIsUpToDate: (value: boolean) => void;
+  saveUserSettings: (newSettings: Partial<Settings>) => Promise<void>;
+  settings: Settings | undefined;
+}
+
+const SettingsContext = React.createContext<SettingsContextType | undefined>(
+  undefined,
+);
+
+interface SettingsProviderProps {
+  children: React.ReactNode;
+}
+
+export function SettingsProvider({ children }: SettingsProviderProps) {
+  const { data: userSettings } = useSettings();
+  const { mutateAsync: saveSettings } = useSaveSettings();
+
+  const [isUpToDate, setIsUpToDate] = React.useState(settingsAreUpToDate());
+
+  const saveUserSettings = async (newSettings: Partial<Settings>) => {
+    const updatedSettings: Partial<Settings> = {
+      ...userSettings,
+      ...newSettings,
+    };
+    await saveSettings(updatedSettings, {
+      onSuccess: () => {
+        if (!isUpToDate) {
+          localStorage.setItem(
+            "SETTINGS_VERSION",
+            LATEST_SETTINGS_VERSION.toString(),
+          );
+          setIsUpToDate(true);
+        }
+      },
+    });
+  };
+
+  const value = React.useMemo(
+    () => ({
+      isUpToDate,
+      setIsUpToDate,
+      saveUserSettings,
+      settings: userSettings,
+    }),
+    [isUpToDate, setIsUpToDate, saveUserSettings, userSettings],
+  );
+
+  return <SettingsContext value={value}>{children}</SettingsContext>;
+}
+
+export function useCurrentSettings() {
+  const context = React.useContext(SettingsContext);
+  if (context === undefined) {
+    throw new Error(
+      "useCurrentSettings must be used within a SettingsProvider",
+    );
+  }
+  return context;
+}

frontend/src/context/settings-up-to-date-context.tsx

@@ -1,40 +0,0 @@
-import React from "react";
-import { settingsAreUpToDate } from "#/services/settings";
-
-interface SettingsUpToDateContextType {
-  isUpToDate: boolean;
-  setIsUpToDate: (value: boolean) => void;
-}
-
-const SettingsUpToDateContext = React.createContext<
-  SettingsUpToDateContextType | undefined
->(undefined);
-
-interface SettingsUpToDateProviderProps {
-  children: React.ReactNode;
-}
-
-export function SettingsUpToDateProvider({
-  children,
-}: SettingsUpToDateProviderProps) {
-  const [isUpToDate, setIsUpToDate] = React.useState(settingsAreUpToDate());
-
-  const value = React.useMemo(
-    () => ({ isUpToDate, setIsUpToDate }),
-    [isUpToDate, setIsUpToDate],
-  );
-
-  return (
-    <SettingsUpToDateContext value={value}>{children}</SettingsUpToDateContext>
-  );
-}
-
-export function useSettingsUpToDate() {
-  const context = React.useContext(SettingsUpToDateContext);
-  if (context === undefined) {
-    throw new Error(
-      "useSettingsUpToDate must be used within a SettingsUpToDateProvider",
-    );
-  }
-  return context;
-}

frontend/src/entry.client.tsx

@@ -20,7 +20,7 @@ import toast from "react-hot-toast";
 import store from "./store";
 import { useConfig } from "./hooks/query/use-config";
 import { AuthProvider } from "./context/auth-context";
-import { SettingsUpToDateProvider } from "./context/settings-up-to-date-context";
+import { SettingsProvider } from "./context/settings-context";
 
 function PosthogInit() {
   const { data: config } = useConfig();
@@ -79,12 +79,12 @@ prepareApp().then(() =>
       <StrictMode>
         <Provider store={store}>
           <AuthProvider>
-            <SettingsUpToDateProvider>
-              <QueryClientProvider client={queryClient}>
+            <QueryClientProvider client={queryClient}>
+              <SettingsProvider>
                 <HydratedRouter />
                 <PosthogInit />
-              </QueryClientProvider>
-            </SettingsUpToDateProvider>
+              </SettingsProvider>
+            </QueryClientProvider>
           </AuthProvider>
         </Provider>
       </StrictMode>,

frontend/src/hooks/mutation/use-save-settings.ts

@@ -1,12 +1,6 @@
 import { useMutation, useQueryClient } from "@tanstack/react-query";
-import {
-  ApiSettings,
-  DEFAULT_SETTINGS,
-  LATEST_SETTINGS_VERSION,
-  Settings,
-} from "#/services/settings";
+import { ApiSettings, DEFAULT_SETTINGS, Settings } from "#/services/settings";
 import OpenHands from "#/api/open-hands";
-import { useSettingsUpToDate } from "#/context/settings-up-to-date-context";
 
 const saveSettingsMutationFn = async (settings: Partial<Settings>) => {
   const apiSettings: Partial<ApiSettings> = {
@@ -24,19 +18,11 @@ const saveSettingsMutationFn = async (settings: Partial<Settings>) => {
 
 export const useSaveSettings = () => {
   const queryClient = useQueryClient();
-  const { isUpToDate, setIsUpToDate } = useSettingsUpToDate();
 
   return useMutation({
     mutationFn: saveSettingsMutationFn,
     onSuccess: async () => {
       await queryClient.invalidateQueries({ queryKey: ["settings"] });
-      if (!isUpToDate) {
-        localStorage.setItem(
-          "SETTINGS_VERSION",
-          LATEST_SETTINGS_VERSION.toString(),
-        );
-        setIsUpToDate(true);
-      }
     },
   });
 };

frontend/src/hooks/query/use-settings.ts

@@ -39,7 +39,6 @@ export const useSettings = () => {
   const query = useQuery({
     queryKey: ["settings"],
     queryFn: getSettingsQueryFn,
-    initialData: DEFAULT_SETTINGS,
   });
 
   React.useEffect(() => {

frontend/src/hooks/use-maybe-migrate-settings.ts

@@ -1,7 +1,7 @@
 // Sometimes we ship major changes, like a new default agent.
 
 import React from "react";
-import { useSettingsUpToDate } from "#/context/settings-up-to-date-context";
+import { useCurrentSettings } from "#/context/settings-context";
 import {
   getCurrentSettingsVersion,
   DEFAULT_SETTINGS,
@@ -12,7 +12,7 @@ import { useSaveSettings } from "./mutation/use-save-settings";
 // In this case, we may want to override a previous choice made by the user.
 export const useMaybeMigrateSettings = () => {
   const { mutateAsync: saveSettings } = useSaveSettings();
-  const { isUpToDate } = useSettingsUpToDate();
+  const { isUpToDate } = useCurrentSettings();
 
   const maybeMigrateSettings = async () => {
     const currentVersion = getCurrentSettingsVersion();

frontend/src/mocks/handlers.ts

@@ -6,7 +6,7 @@ import {
 } from "#/api/open-hands.types";
 import { DEFAULT_SETTINGS } from "#/services/settings";
 
-const userPreferences = {
+export const MOCK_USER_PREFERENCES = {
   settings: {
     llm_model: DEFAULT_SETTINGS.LLM_MODEL,
     llm_base_url: DEFAULT_SETTINGS.LLM_BASE_URL,
@@ -169,14 +169,14 @@ export const handlers = [
     return HttpResponse.json(config);
   }),
   http.get("/api/settings", async () =>
-    HttpResponse.json(userPreferences.settings),
+    HttpResponse.json(MOCK_USER_PREFERENCES.settings),
   ),
   http.post("/api/settings", async ({ request }) => {
     const body = await request.json();
 
     if (body) {
-      userPreferences.settings = {
-        ...userPreferences.settings,
+      MOCK_USER_PREFERENCES.settings = {
+        ...MOCK_USER_PREFERENCES.settings,
         // @ts-expect-error - We know this is a settings object
         ...body,
       };

frontend/src/routes/_oh.app/route.tsx

@@ -176,13 +176,15 @@ function AppContent() {
 
           <Controls
             setSecurityOpen={onSecurityModalOpen}
-            showSecurityLock={!!settings.SECURITY_ANALYZER}
-          />
-          <Security
-            isOpen={securityModalIsOpen}
-            onOpenChange={onSecurityModalOpenChange}
-            securityAnalyzer={settings.SECURITY_ANALYZER}
+            showSecurityLock={!!settings?.SECURITY_ANALYZER}
           />
+          {settings && (
+            <Security
+              isOpen={securityModalIsOpen}
+              onOpenChange={onSecurityModalOpenChange}
+              securityAnalyzer={settings.SECURITY_ANALYZER}
+            />
+          )}
         </div>
       </EventHandler>
     </WsClientProvider>

frontend/src/routes/_oh/route.tsx

@@ -63,10 +63,10 @@ export default function MainApp() {
   });
 
   React.useEffect(() => {
-    if (settings.LANGUAGE) {
+    if (settings?.LANGUAGE) {
       i18n.changeLanguage(settings.LANGUAGE);
     }
-  }, [settings.LANGUAGE]);
+  }, [settings?.LANGUAGE]);
 
   const isInWaitlist =
     !isFetchingAuth && !isAuthed && config.data?.APP_MODE === "saas";

frontend/test-utils.tsx

@@ -11,7 +11,7 @@ import { vi } from "vitest";
 import { AppStore, RootState, rootReducer } from "./src/store";
 import { AuthProvider } from "#/context/auth-context";
 import { ConversationProvider } from "#/context/conversation-context";
-import { SettingsUpToDateProvider } from "#/context/settings-up-to-date-context";
+import { SettingsProvider } from "#/context/settings-context";
 
 // Mock useParams before importing components
 vi.mock("react-router", async () => {
@@ -67,19 +67,19 @@ export function renderWithProviders(
     return (
       <Provider store={store}>
         <AuthProvider>
-          <SettingsUpToDateProvider>
-            <QueryClientProvider
-              client={
-                new QueryClient({
-                  defaultOptions: { queries: { retry: false } },
-                })
-              }
-            >
+          <QueryClientProvider
+            client={
+              new QueryClient({
+                defaultOptions: { queries: { retry: false } },
+              })
+            }
+          >
+            <SettingsProvider>
               <ConversationProvider>
                 <I18nextProvider i18n={i18n}>{children}</I18nextProvider>
               </ConversationProvider>
-            </QueryClientProvider>
-          </SettingsUpToDateProvider>
+            </SettingsProvider>
+          </QueryClientProvider>
         </AuthProvider>
       </Provider>
     );