From 4de0a27ed2356a56b5725e9a00fa0a2e22b263fd Mon Sep 17 00:00:00 2001 From: "sp.wack" <83104063+amanape@users.noreply.github.com> Date: Tue, 31 Dec 2024 13:40:51 +0400 Subject: [PATCH] test(frontend): Settings logic and new API key UI (#5873) --- frontend/src/api/open-hands.ts | 19 ++++- .../components/features/sidebar/sidebar.tsx | 18 ++-- .../shared/buttons/settings-button.tsx | 7 +- .../shared/inputs/api-key-input.tsx | 24 ++++-- .../account-settings-form.tsx | 4 +- .../account-settings-modal.tsx | 4 +- .../modals/confirmation-modals/base-modal.tsx | 10 ++- .../confirmation-modals/danger-modal.tsx | 10 ++- .../shared/modals/settings/model-selector.tsx | 14 +++- .../shared/modals/settings/settings-form.tsx | 15 ++-- .../shared/modals/settings/settings-modal.tsx | 6 +- frontend/src/context/settings-context.tsx | 68 --------------- .../context/settings-up-to-date-context.tsx | 42 ++++++++++ frontend/src/entry.client.tsx | 14 ++-- .../hooks/mutation/use-create-conversation.ts | 4 +- .../src/hooks/mutation/use-save-settings.ts | 41 +++++++++ frontend/src/hooks/query/use-settings.ts | 39 +++++++++ .../src/hooks/use-maybe-migrate-settings.ts | 54 ++++++++++++ frontend/src/mocks/handlers.ts | 66 +++++++++++---- frontend/src/routes/_oh.app/route.tsx | 5 +- frontend/src/routes/_oh/route.tsx | 13 ++- frontend/src/services/settings.ts | 83 ++----------------- frontend/src/utils/settings-utils.ts | 23 +---- frontend/test-utils.tsx | 14 ++-- frontend/tests/settings.spec.ts | 73 ++++++++++++++++ openhands/server/routes/settings.py | 10 ++- 26 files changed, 433 insertions(+), 247 deletions(-) delete mode 100644 frontend/src/context/settings-context.tsx create mode 100644 frontend/src/context/settings-up-to-date-context.tsx create mode 100644 frontend/src/hooks/mutation/use-save-settings.ts create mode 100644 frontend/src/hooks/query/use-settings.ts create mode 100644 frontend/src/hooks/use-maybe-migrate-settings.ts create mode 100644 frontend/tests/settings.spec.ts diff --git a/frontend/src/api/open-hands.ts b/frontend/src/api/open-hands.ts index a65301f8a1..1d1bd0dfa8 100644 --- a/frontend/src/api/open-hands.ts +++ b/frontend/src/api/open-hands.ts @@ -11,7 +11,7 @@ import { Conversation, } from "./open-hands.types"; import { openHands } from "./open-hands-axios"; -import { Settings } from "#/services/settings"; +import { ApiSettings, Settings } from "#/services/settings"; class OpenHands { /** @@ -297,6 +297,23 @@ class OpenHands { }); return data; } + + /** + * Get the settings from the server or use the default settings if not found + */ + static async getSettings(): Promise { + const { data } = await openHands.get("/api/settings"); + return data; + } + + /** + * Save the settings to the server. Only valid settings are saved. + * @param settings - the settings to save + */ + static async saveSettings(settings: Partial): Promise { + const data = await openHands.post("/api/settings", settings); + return data.status === 200; + } } export default OpenHands; diff --git a/frontend/src/components/features/sidebar/sidebar.tsx b/frontend/src/components/features/sidebar/sidebar.tsx index 85a004f0a3..3d2e9a3e60 100644 --- a/frontend/src/components/features/sidebar/sidebar.tsx +++ b/frontend/src/components/features/sidebar/sidebar.tsx @@ -2,7 +2,6 @@ import React from "react"; import { useLocation } from "react-router"; import FolderIcon from "#/icons/docs.svg?react"; import { useAuth } from "#/context/auth-context"; -import { useSettings } from "#/context/settings-context"; import { useGitHubUser } from "#/hooks/query/use-github-user"; import { useIsAuthed } from "#/hooks/query/use-is-authed"; import { UserActions } from "./user-actions"; @@ -14,18 +13,19 @@ import { LoadingSpinner } from "#/components/shared/loading-spinner"; import { AccountSettingsModal } from "#/components/shared/modals/account-settings/account-settings-modal"; import { ExitProjectConfirmationModal } from "#/components/shared/modals/exit-project-confirmation-modal"; import { SettingsModal } from "#/components/shared/modals/settings/settings-modal"; +import { useSettingsUpToDate } from "#/context/settings-up-to-date-context"; +import { useSettings } from "#/hooks/query/use-settings"; import { ConversationPanel } from "../conversation-panel/conversation-panel"; import { cn } from "#/utils/utils"; import { MULTI_CONVO_UI_IS_ENABLED } from "#/utils/constants"; export function Sidebar() { const location = useLocation(); - const user = useGitHubUser(); const { data: isAuthed } = useIsAuthed(); - const { logout } = useAuth(); - const { settingsAreUpToDate } = useSettings(); + const { data: settings, isError: settingsIsError } = useSettings(); + const { isUpToDate: settingsAreUpToDate } = useSettingsUpToDate(); const [accountSettingsModalOpen, setAccountSettingsModalOpen] = React.useState(false); @@ -109,9 +109,13 @@ export function Sidebar() { {accountSettingsModalOpen && ( )} - {showSettingsModal && ( - setSettingsModalIsOpen(false)} /> - )} + {settingsIsError || + (showSettingsModal && ( + setSettingsModalIsOpen(false)} + /> + ))} {startNewProjectModalIsOpen && ( setStartNewProjectModalIsOpen(false)} diff --git a/frontend/src/components/shared/buttons/settings-button.tsx b/frontend/src/components/shared/buttons/settings-button.tsx index 6c9e20702b..59ed58ed27 100644 --- a/frontend/src/components/shared/buttons/settings-button.tsx +++ b/frontend/src/components/shared/buttons/settings-button.tsx @@ -7,7 +7,12 @@ interface SettingsButtonProps { export function SettingsButton({ onClick }: SettingsButtonProps) { return ( - + ); diff --git a/frontend/src/components/shared/inputs/api-key-input.tsx b/frontend/src/components/shared/inputs/api-key-input.tsx index ea6e553adb..7bdafb90bd 100644 --- a/frontend/src/components/shared/inputs/api-key-input.tsx +++ b/frontend/src/components/shared/inputs/api-key-input.tsx @@ -1,27 +1,37 @@ -import { Input } from "@nextui-org/react"; +import { Input, Tooltip } from "@nextui-org/react"; import { useTranslation } from "react-i18next"; +import { FaCheckCircle, FaExclamationCircle } from "react-icons/fa"; import { I18nKey } from "#/i18n/declaration"; interface APIKeyInputProps { isDisabled: boolean; - defaultValue: string; + isSet: boolean; } -export function APIKeyInput({ isDisabled, defaultValue }: APIKeyInputProps) { +export function APIKeyInput({ isDisabled, isSet }: APIKeyInputProps) { const { t } = useTranslation(); return (
- + + + void; @@ -30,7 +30,7 @@ export function AccountSettingsForm({ }: AccountSettingsFormProps) { const { gitHubToken, setGitHubToken, logout } = useAuth(); const { data: config } = useConfig(); - const { saveSettings } = useSettings(); + const { mutate: saveSettings } = useSaveSettings(); const { t } = useTranslation(); const handleSubmit = (event: React.FormEvent) => { diff --git a/frontend/src/components/shared/modals/account-settings/account-settings-modal.tsx b/frontend/src/components/shared/modals/account-settings/account-settings-modal.tsx index d8c1ce47f7..0de5a91458 100644 --- a/frontend/src/components/shared/modals/account-settings/account-settings-modal.tsx +++ b/frontend/src/components/shared/modals/account-settings/account-settings-modal.tsx @@ -1,5 +1,5 @@ -import { useSettings } from "#/context/settings-context"; import { useGitHubUser } from "#/hooks/query/use-github-user"; +import { useSettings } from "#/hooks/query/use-settings"; import { ModalBackdrop } from "../modal-backdrop"; import { AccountSettingsForm } from "./account-settings-form"; @@ -9,7 +9,7 @@ interface AccountSettingsModalProps { export function AccountSettingsModal({ onClose }: AccountSettingsModalProps) { const user = useGitHubUser(); - const { settings } = useSettings(); + const { data: settings } = useSettings(); // FIXME: Bad practice to use localStorage directly const analyticsConsent = localStorage.getItem("analytics-consent"); diff --git a/frontend/src/components/shared/modals/confirmation-modals/base-modal.tsx b/frontend/src/components/shared/modals/confirmation-modals/base-modal.tsx index 2cb79b45c8..c5b4e3f255 100644 --- a/frontend/src/components/shared/modals/confirmation-modals/base-modal.tsx +++ b/frontend/src/components/shared/modals/confirmation-modals/base-modal.tsx @@ -35,14 +35,20 @@ export function BaseModalDescription({ } interface BaseModalProps { + testId?: string; title: string; description: string; buttons: ButtonConfig[]; } -export function BaseModal({ title, description, buttons }: BaseModalProps) { +export function BaseModal({ + testId, + title, + description, + buttons, +}: BaseModalProps) { return ( - +
diff --git a/frontend/src/components/shared/modals/confirmation-modals/danger-modal.tsx b/frontend/src/components/shared/modals/confirmation-modals/danger-modal.tsx index fa6d468d00..98f4a7155f 100644 --- a/frontend/src/components/shared/modals/confirmation-modals/danger-modal.tsx +++ b/frontend/src/components/shared/modals/confirmation-modals/danger-modal.tsx @@ -1,6 +1,8 @@ import { BaseModal } from "./base-modal"; interface DangerModalProps { + testId?: string; + title: string; description: string; @@ -10,9 +12,15 @@ interface DangerModalProps { }; } -export function DangerModal({ title, description, buttons }: DangerModalProps) { +export function DangerModal({ + testId, + title, + description, + buttons, +}: DangerModalProps) { return ( VERIFIED_PROVIDERS.includes(provider)) .map((provider) => ( - + {mapProvider(provider)} ))} @@ -113,6 +118,7 @@ export function ModelSelector({ LLM Model !VERIFIED_MODELS.includes(model)) .map((model) => ( - + {model} ))} diff --git a/frontend/src/components/shared/modals/settings/settings-form.tsx b/frontend/src/components/shared/modals/settings/settings-form.tsx index 918046178f..7aba4856e2 100644 --- a/frontend/src/components/shared/modals/settings/settings-form.tsx +++ b/frontend/src/components/shared/modals/settings/settings-form.tsx @@ -9,7 +9,6 @@ import { DangerModal } from "../confirmation-modals/danger-modal"; import { I18nKey } from "#/i18n/declaration"; import { extractSettings, saveSettingsView } from "#/utils/settings-utils"; import { useEndSession } from "#/hooks/use-end-session"; -import { useSettings } from "#/context/settings-context"; import { ModalButton } from "../../buttons/modal-button"; import { AdvancedOptionSwitch } from "../../inputs/advanced-option-switch"; import { AgentInput } from "../../inputs/agent-input"; @@ -20,6 +19,7 @@ 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"; interface SettingsFormProps { disabled?: boolean; @@ -38,7 +38,7 @@ export function SettingsForm({ securityAnalyzers, onClose, }: SettingsFormProps) { - const { saveSettings } = useSettings(); + const { mutateAsync: saveSettings } = useSaveSettings(); const endSession = useEndSession(); const location = useLocation(); @@ -82,7 +82,6 @@ export function SettingsForm({ const resetOngoingSession = () => { if (location.pathname.startsWith("/conversations/")) { endSession(); - onClose(); } }; @@ -92,7 +91,7 @@ export function SettingsForm({ const newSettings = extractSettings(formData); saveSettingsView(isUsingAdvancedOptions ? "advanced" : "basic"); - await saveSettings(newSettings); + await saveSettings(newSettings, { onSuccess: onClose }); resetOngoingSession(); posthog.capture("settings_saved", { @@ -102,11 +101,9 @@ export function SettingsForm({ }; const handleConfirmResetSettings = async () => { - await saveSettings(getDefaultSettings()); + await saveSettings(getDefaultSettings(), { onSuccess: onClose }); resetOngoingSession(); posthog.capture("settings_reset"); - - onClose(); }; const handleConfirmEndSession = () => { @@ -122,7 +119,6 @@ export function SettingsForm({ setConfirmEndSessionModalOpen(true); } else { handleFormSubmission(formData); - onClose(); } }; @@ -165,7 +161,7 @@ export function SettingsForm({ {showAdvancedOptions && ( @@ -221,6 +217,7 @@ export function SettingsForm({ {confirmResetDefaultsModalOpen && ( void; } -export function SettingsModal({ onClose }: SettingsModalProps) { - const { settings } = useSettings(); +export function SettingsModal({ onClose, settings }: SettingsModalProps) { const aiConfigOptions = useAIConfigOptions(); return ( diff --git a/frontend/src/context/settings-context.tsx b/frontend/src/context/settings-context.tsx deleted file mode 100644 index 3d2c426dc0..0000000000 --- a/frontend/src/context/settings-context.tsx +++ /dev/null @@ -1,68 +0,0 @@ -import React from "react"; -import posthog from "posthog-js"; -import { useQuery, useQueryClient } from "@tanstack/react-query"; -import { - getSettings, - Settings, - saveSettings, - settingsAreUpToDate as checkIfSettingsAreUpToDate, - DEFAULT_SETTINGS, -} from "#/services/settings"; - -interface SettingsContextType { - settings: Settings; - settingsAreUpToDate: boolean; - saveSettings: (settings: Partial) => void; -} - -const SettingsContext = React.createContext( - undefined, -); - -const SETTINGS_QUERY_KEY = ["settings"]; - -function SettingsProvider({ children }: React.PropsWithChildren) { - const { data: settings } = useQuery({ - queryKey: SETTINGS_QUERY_KEY, - queryFn: getSettings, - initialData: DEFAULT_SETTINGS, - }); - - const [settingsAreUpToDate, setSettingsAreUpToDate] = React.useState( - checkIfSettingsAreUpToDate(), - ); - const queryClient = useQueryClient(); - - const handleSaveSettings = async (newSettings: Partial) => { - await saveSettings(newSettings); - queryClient.invalidateQueries({ queryKey: SETTINGS_QUERY_KEY }); - setSettingsAreUpToDate(checkIfSettingsAreUpToDate()); - }; - - React.useEffect(() => { - if (settings?.LLM_API_KEY) { - posthog.capture("user_activated"); - } - }, [settings?.LLM_API_KEY]); - - const value = React.useMemo( - () => ({ - settings, - settingsAreUpToDate, - saveSettings: handleSaveSettings, - }), - [settings, settingsAreUpToDate], - ); - - return {children}; -} - -function useSettings() { - const context = React.useContext(SettingsContext); - if (context === undefined) { - throw new Error("useSettings must be used within a SettingsProvider"); - } - return context; -} - -export { SettingsProvider, useSettings }; diff --git a/frontend/src/context/settings-up-to-date-context.tsx b/frontend/src/context/settings-up-to-date-context.tsx new file mode 100644 index 0000000000..93e8f71556 --- /dev/null +++ b/frontend/src/context/settings-up-to-date-context.tsx @@ -0,0 +1,42 @@ +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 ( + + {children} + + ); +} + +export function useSettingsUpToDate() { + const context = React.useContext(SettingsUpToDateContext); + if (context === undefined) { + throw new Error( + "useSettingsUpToDate must be used within a SettingsUpToDateProvider", + ); + } + return context; +} diff --git a/frontend/src/entry.client.tsx b/frontend/src/entry.client.tsx index 78f2e893b1..06ebe25a82 100644 --- a/frontend/src/entry.client.tsx +++ b/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 { SettingsProvider } from "./context/settings-context"; +import { SettingsUpToDateProvider } from "./context/settings-up-to-date-context"; function PosthogInit() { const { data: config } = useConfig(); @@ -71,14 +71,14 @@ prepareApp().then(() => document, - - - + + + - - - + + + , ); diff --git a/frontend/src/hooks/mutation/use-create-conversation.ts b/frontend/src/hooks/mutation/use-create-conversation.ts index 91d4c83918..1a9c7e3a79 100644 --- a/frontend/src/hooks/mutation/use-create-conversation.ts +++ b/frontend/src/hooks/mutation/use-create-conversation.ts @@ -6,13 +6,13 @@ import OpenHands from "#/api/open-hands"; import { setInitialQuery } from "#/state/initial-query-slice"; import { RootState } from "#/store"; import { useAuth } from "#/context/auth-context"; -import { useSettings } from "#/context/settings-context"; +import { useSettings } from "../query/use-settings"; export const useCreateConversation = () => { const navigate = useNavigate(); const dispatch = useDispatch(); const { gitHubToken } = useAuth(); - const { settings } = useSettings(); + const { data: settings } = useSettings(); const queryClient = useQueryClient(); const { selectedRepository, files } = useSelector( diff --git a/frontend/src/hooks/mutation/use-save-settings.ts b/frontend/src/hooks/mutation/use-save-settings.ts new file mode 100644 index 0000000000..2fb998cb4c --- /dev/null +++ b/frontend/src/hooks/mutation/use-save-settings.ts @@ -0,0 +1,41 @@ +import { useMutation, useQueryClient } from "@tanstack/react-query"; +import { + ApiSettings, + LATEST_SETTINGS_VERSION, + Settings, +} from "#/services/settings"; +import OpenHands from "#/api/open-hands"; +import { useSettingsUpToDate } from "#/context/settings-up-to-date-context"; + +const saveSettingsMutationFn = async (settings: Partial) => { + const apiSettings: Partial = { + llm_model: settings.LLM_MODEL, + llm_base_url: settings.LLM_BASE_URL, + agent: settings.AGENT, + language: settings.LANGUAGE, + confirmation_mode: settings.CONFIRMATION_MODE, + security_analyzer: settings.SECURITY_ANALYZER, + llm_api_key: settings.LLM_API_KEY, + }; + + await OpenHands.saveSettings(apiSettings); +}; + +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); + } + }, + }); +}; diff --git a/frontend/src/hooks/query/use-settings.ts b/frontend/src/hooks/query/use-settings.ts new file mode 100644 index 0000000000..796c0cab18 --- /dev/null +++ b/frontend/src/hooks/query/use-settings.ts @@ -0,0 +1,39 @@ +import { useQuery } from "@tanstack/react-query"; +import React from "react"; +import posthog from "posthog-js"; +import { DEFAULT_SETTINGS, getLocalStorageSettings } from "#/services/settings"; +import OpenHands from "#/api/open-hands"; + +const getSettingsQueryFn = async () => { + const apiSettings = await OpenHands.getSettings(); + + if (apiSettings !== null) { + return { + LLM_MODEL: apiSettings.llm_model, + LLM_BASE_URL: apiSettings.llm_base_url, + AGENT: apiSettings.agent, + LANGUAGE: apiSettings.language, + CONFIRMATION_MODE: apiSettings.confirmation_mode, + SECURITY_ANALYZER: apiSettings.security_analyzer, + LLM_API_KEY: apiSettings.llm_api_key, + }; + } + + return getLocalStorageSettings(); +}; + +export const useSettings = () => { + const query = useQuery({ + queryKey: ["settings"], + queryFn: getSettingsQueryFn, + initialData: DEFAULT_SETTINGS, + }); + + React.useEffect(() => { + if (query.data?.LLM_API_KEY) { + posthog.capture("user_activated"); + } + }, [query.data?.LLM_API_KEY]); + + return query; +}; diff --git a/frontend/src/hooks/use-maybe-migrate-settings.ts b/frontend/src/hooks/use-maybe-migrate-settings.ts new file mode 100644 index 0000000000..d2bb49f3c1 --- /dev/null +++ b/frontend/src/hooks/use-maybe-migrate-settings.ts @@ -0,0 +1,54 @@ +// Sometimes we ship major changes, like a new default agent. + +import React from "react"; +import { useAuth } from "#/context/auth-context"; +import { useSettingsUpToDate } from "#/context/settings-up-to-date-context"; +import { + DEFAULT_SETTINGS, + getCurrentSettingsVersion, + getLocalStorageSettings, +} from "#/services/settings"; +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 { logout } = useAuth(); + const { mutateAsync: saveSettings } = useSaveSettings(); + const { isUpToDate } = useSettingsUpToDate(); + + const maybeMigrateSettings = async () => { + const currentVersion = getCurrentSettingsVersion(); + + if (currentVersion < 1) { + localStorage.setItem("AGENT", DEFAULT_SETTINGS.AGENT); + } + if (currentVersion < 2) { + const customModel = localStorage.getItem("CUSTOM_LLM_MODEL"); + if (customModel) { + localStorage.setItem("LLM_MODEL", customModel); + } + localStorage.removeItem("CUSTOM_LLM_MODEL"); + localStorage.removeItem("USING_CUSTOM_MODEL"); + } + if (currentVersion < 3) { + localStorage.removeItem("token"); + } + + if (currentVersion < 4) { + logout(); + } + + // Only save settings if user already previously saved settings + // That way we avoid setting defaults for new users too early + if (currentVersion !== 0 && currentVersion < 5) { + const localSettings = getLocalStorageSettings(); + await saveSettings(localSettings); + } + }; + + React.useEffect(() => { + if (!isUpToDate) { + maybeMigrateSettings(); + } + }, []); +}; diff --git a/frontend/src/mocks/handlers.ts b/frontend/src/mocks/handlers.ts index d360e88966..0ad9074124 100644 --- a/frontend/src/mocks/handlers.ts +++ b/frontend/src/mocks/handlers.ts @@ -1,5 +1,18 @@ import { delay, http, HttpResponse } from "msw"; -import { Conversation } from "#/api/open-hands.types"; +import { GetConfigResponse, Conversation } from "#/api/open-hands.types"; +import { DEFAULT_SETTINGS } from "#/services/settings"; + +const userPreferences = { + settings: { + llm_model: DEFAULT_SETTINGS.LLM_MODEL, + llm_base_url: DEFAULT_SETTINGS.LLM_BASE_URL, + llm_api_key: DEFAULT_SETTINGS.LLM_API_KEY, + agent: DEFAULT_SETTINGS.AGENT, + language: DEFAULT_SETTINGS.LANGUAGE, + confirmation_mode: DEFAULT_SETTINGS.CONFIRMATION_MODE, + security_analyzer: DEFAULT_SETTINGS.SECURITY_ANALYZER, + }, +}; const conversations: Conversation[] = [ { @@ -35,24 +48,22 @@ const CONVERSATIONS = new Map( ); const openHandsHandlers = [ - http.get("/api/options/models", async () => { - await delay(); - return HttpResponse.json([ + http.get("/api/options/models", async () => + HttpResponse.json([ "gpt-3.5-turbo", "gpt-4o", "anthropic/claude-3.5", - ]); - }), + "anthropic/claude-3-5-sonnet-20241022", + ]), + ), - http.get("/api/options/agents", async () => { - await delay(); - return HttpResponse.json(["CodeActAgent", "CoActAgent"]); - }), + http.get("/api/options/agents", async () => + HttpResponse.json(["CodeActAgent", "CoActAgent"]), + ), - http.get("/api/options/security-analyzers", async () => { - await delay(); - return HttpResponse.json(["mock-invariant"]); - }), + http.get("/api/options/security-analyzers", async () => + HttpResponse.json(["mock-invariant"]), + ), http.get( "http://localhost:3001/api/conversations/:conversationId/list-files", @@ -137,6 +148,33 @@ export const handlers = [ http.post("https://us.i.posthog.com/e", async () => HttpResponse.json(null, { status: 200 }), ), + http.get("/api/options/config", () => { + const config: GetConfigResponse = { + APP_MODE: "oss", + GITHUB_CLIENT_ID: "fake-github-client-id", + POSTHOG_CLIENT_KEY: "fake-posthog-client-key", + }; + + return HttpResponse.json(config); + }), + http.get("/api/settings", async () => + HttpResponse.json(userPreferences.settings), + ), + http.post("/api/settings", async ({ request }) => { + const body = await request.json(); + + if (body) { + userPreferences.settings = { + ...userPreferences.settings, + // @ts-expect-error - We know this is a settings object + ...body, + }; + + return HttpResponse.json(null, { status: 200 }); + } + + return HttpResponse.json(null, { status: 400 }); + }), http.post("/api/authenticate", async () => HttpResponse.json({ message: "Authenticated" }), diff --git a/frontend/src/routes/_oh.app/route.tsx b/frontend/src/routes/_oh.app/route.tsx index 6f63085b52..733d71bc32 100644 --- a/frontend/src/routes/_oh.app/route.tsx +++ b/frontend/src/routes/_oh.app/route.tsx @@ -22,7 +22,6 @@ import { WsClientProvider } from "#/context/ws-client-provider"; import { EventHandler } from "./event-handler"; import { useLatestRepoCommit } from "#/hooks/query/use-latest-repo-commit"; import { useAuth } from "#/context/auth-context"; -import { useSettings } from "#/context/settings-context"; import { useConversationConfig } from "#/hooks/query/use-conversation-config"; import { Container } from "#/components/layout/container"; import { @@ -34,14 +33,16 @@ import { useEndSession } from "#/hooks/use-end-session"; import { useUserConversation } from "#/hooks/query/get-conversation-permissions"; import { CountBadge } from "#/components/layout/count-badge"; import { TerminalStatusLabel } from "#/components/features/terminal/terminal-status-label"; +import { useSettings } from "#/hooks/query/use-settings"; import { MULTI_CONVO_UI_IS_ENABLED } from "#/utils/constants"; function AppContent() { const { gitHubToken } = useAuth(); + const { data: settings } = useSettings(); + const endSession = useEndSession(); const [width, setWidth] = React.useState(window.innerWidth); - const { settings } = useSettings(); const { conversationId } = useConversation(); const dispatch = useDispatch(); diff --git a/frontend/src/routes/_oh/route.tsx b/frontend/src/routes/_oh/route.tsx index 9be13d8ad8..47e5ffe52e 100644 --- a/frontend/src/routes/_oh/route.tsx +++ b/frontend/src/routes/_oh/route.tsx @@ -4,12 +4,12 @@ import i18n from "#/i18n"; import { useGitHubAuthUrl } from "#/hooks/use-github-auth-url"; import { useIsAuthed } from "#/hooks/query/use-is-authed"; import { useAuth } from "#/context/auth-context"; -import { useSettings } from "#/context/settings-context"; -import { updateSettingsVersion } from "#/utils/settings-utils"; import { useConfig } from "#/hooks/query/use-config"; import { Sidebar } from "#/components/features/sidebar/sidebar"; import { WaitlistModal } from "#/components/features/waitlist/waitlist-modal"; import { AnalyticsConsentFormModal } from "#/components/features/analytics/analytics-consent-form-modal"; +import { useSettings } from "#/hooks/query/use-settings"; +import { useMaybeMigrateSettings } from "#/hooks/use-maybe-migrate-settings"; export function ErrorBoundary() { const error = useRouteError(); @@ -44,9 +44,10 @@ export function ErrorBoundary() { } export default function MainApp() { + useMaybeMigrateSettings(); + const { gitHubToken } = useAuth(); - const { settings } = useSettings(); - const { logout } = useAuth(); + const { data: settings } = useSettings(); const [consentFormIsOpen, setConsentFormIsOpen] = React.useState( !localStorage.getItem("analytics-consent"), @@ -67,10 +68,6 @@ export default function MainApp() { } }, [settings.LANGUAGE]); - React.useEffect(() => { - updateSettingsVersion(logout); - }, []); - const isInWaitlist = !isFetchingAuth && !isAuthed && config.data?.APP_MODE === "saas"; diff --git a/frontend/src/services/settings.ts b/frontend/src/services/settings.ts index a0dbb775f5..b42d7f1042 100644 --- a/frontend/src/services/settings.ts +++ b/frontend/src/services/settings.ts @@ -1,5 +1,3 @@ -import { openHands } from "#/api/open-hands-axios"; - export const LATEST_SETTINGS_VERSION = 5; export type Settings = { @@ -46,6 +44,11 @@ export const settingsAreUpToDate = () => getCurrentSettingsVersion() === LATEST_SETTINGS_VERSION; // TODO: localStorage settings are deprecated. Remove this after 1/31/2025 +/** + * Get the settings from local storage + * @returns the settings from local storage + * @deprecated + */ export const getLocalStorageSettings = (): Settings => { const llmModel = localStorage.getItem("LLM_MODEL"); const baseUrl = localStorage.getItem("LLM_BASE_URL"); @@ -66,83 +69,7 @@ export const getLocalStorageSettings = (): Settings => { }; }; -/** - * Save the settings to the server. Only valid settings are saved. - * @param settings - the settings to save - */ -export const saveSettings = async ( - settings: Partial, -): Promise => { - try { - const apiSettings = { - llm_model: settings.LLM_MODEL || null, - llm_base_url: settings.LLM_BASE_URL || null, - agent: settings.AGENT || null, - language: settings.LANGUAGE || null, - confirmation_mode: settings.CONFIRMATION_MODE || null, - security_analyzer: settings.SECURITY_ANALYZER || null, - llm_api_key: settings.LLM_API_KEY || null, - }; - - const { data } = await openHands.post("/api/settings", apiSettings); - return data; - } catch (error) { - // Error handled by returning false - return false; - } -}; - -export const maybeMigrateSettings = async (logout: () => void) => { - // 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); - } - if (currentVersion < 2) { - const customModel = localStorage.getItem("CUSTOM_LLM_MODEL"); - if (customModel) { - localStorage.setItem("LLM_MODEL", customModel); - } - localStorage.removeItem("CUSTOM_LLM_MODEL"); - localStorage.removeItem("USING_CUSTOM_MODEL"); - } - if (currentVersion < 3) { - localStorage.removeItem("token"); - } - - if (currentVersion < 4) { - logout(); - } - - if (currentVersion < 5) { - const localSettings = getLocalStorageSettings(); - await saveSettings(localSettings); - } -}; - /** * Get the default settings */ export const getDefaultSettings = (): Settings => DEFAULT_SETTINGS; - -/** - * Get the settings from the server or use the default settings if not found - */ -export const getSettings = async (): Promise => { - const { data: apiSettings } = - await openHands.get("/api/settings"); - if (apiSettings != null) { - return { - LLM_MODEL: apiSettings.llm_model, - LLM_BASE_URL: apiSettings.llm_base_url, - AGENT: apiSettings.agent, - LANGUAGE: apiSettings.language, - CONFIRMATION_MODE: apiSettings.confirmation_mode, - SECURITY_ANALYZER: apiSettings.security_analyzer, - LLM_API_KEY: "", - }; - } - return getLocalStorageSettings(); -}; diff --git a/frontend/src/utils/settings-utils.ts b/frontend/src/utils/settings-utils.ts index bb4780d301..f16d5bb0a3 100644 --- a/frontend/src/utils/settings-utils.ts +++ b/frontend/src/utils/settings-utils.ts @@ -1,9 +1,4 @@ -import { - settingsAreUpToDate, - maybeMigrateSettings, - LATEST_SETTINGS_VERSION, - Settings, -} from "#/services/settings"; +import { Settings } from "#/services/settings"; const extractBasicFormData = (formData: FormData) => { const provider = formData.get("llm-provider")?.toString(); @@ -78,18 +73,4 @@ const saveSettingsView = (view: "basic" | "advanced") => { ); }; -/** - * Updates the settings version in local storage if the current settings are not up to date. - * If the settings are outdated, it attempts to migrate them before updating the version. - */ -const updateSettingsVersion = async (logout: () => void) => { - if (!settingsAreUpToDate()) { - await maybeMigrateSettings(logout); - localStorage.setItem( - "SETTINGS_VERSION", - LATEST_SETTINGS_VERSION.toString(), - ); - } -}; - -export { extractSettings, saveSettingsView, updateSettingsVersion }; +export { extractSettings, saveSettingsView }; diff --git a/frontend/test-utils.tsx b/frontend/test-utils.tsx index 174b8885d7..a2819e8ccf 100644 --- a/frontend/test-utils.tsx +++ b/frontend/test-utils.tsx @@ -10,8 +10,8 @@ import i18n from "i18next"; import { vi } from "vitest"; import { AppStore, RootState, rootReducer } from "./src/store"; import { AuthProvider } from "#/context/auth-context"; -import { SettingsProvider } from "#/context/settings-context"; import { ConversationProvider } from "#/context/conversation-context"; +import { SettingsUpToDateProvider } from "#/context/settings-up-to-date-context"; // Mock useParams before importing components vi.mock("react-router", async () => { @@ -66,15 +66,15 @@ export function renderWithProviders( function Wrapper({ children }: PropsWithChildren) { return ( - - - + + + {children} - - - + + + ); } diff --git a/frontend/tests/settings.spec.ts b/frontend/tests/settings.spec.ts new file mode 100644 index 0000000000..16f5cc5958 --- /dev/null +++ b/frontend/tests/settings.spec.ts @@ -0,0 +1,73 @@ +import test, { expect, Page } from "@playwright/test"; + +test.beforeEach(async ({ page }) => { + await page.goto("/"); + await page.evaluate(() => { + localStorage.setItem("analytics-consent", "true"); + localStorage.setItem("SETTINGS_VERSION", "4"); + }); +}); + +const selectGpt4o = async (page: Page) => { + const aiConfigModal = page.getByTestId("ai-config-modal"); + await expect(aiConfigModal).toBeVisible(); + + const providerSelectElement = aiConfigModal.getByTestId("llm-provider"); + await providerSelectElement.click(); + + const openAiOption = page.getByTestId("provider-item-openai"); + await openAiOption.click(); + + const modelSelectElement = aiConfigModal.getByTestId("llm-model"); + await modelSelectElement.click(); + + const gpt4Option = page.getByText("gpt-4o", { exact: true }); + await gpt4Option.click(); + + return { + aiConfigModal, + providerSelectElement, + modelSelectElement, + }; +}; + +test("change ai config settings", async ({ page }) => { + const { aiConfigModal, modelSelectElement, providerSelectElement } = + await selectGpt4o(page); + + const saveButton = aiConfigModal.getByText("Save"); + await saveButton.click(); + + const settingsButton = page.getByTestId("settings-button"); + await settingsButton.click(); + + await expect(providerSelectElement).toHaveValue("OpenAI"); + await expect(modelSelectElement).toHaveValue("gpt-4o"); +}); + +test("reset to default settings", async ({ page }) => { + const { aiConfigModal } = await selectGpt4o(page); + + const saveButton = aiConfigModal.getByText("Save"); + await saveButton.click(); + + const settingsButton = page.getByTestId("settings-button"); + await settingsButton.click(); + + const resetButton = aiConfigModal.getByText(/reset to defaults/i); + await resetButton.click(); + + const endSessionModal = page.getByTestId("reset-defaults-modal"); + expect(endSessionModal).toBeVisible(); + + const confirmButton = endSessionModal.getByText(/reset to defaults/i); + await confirmButton.click(); + + await settingsButton.click(); + + const providerSelectElement = aiConfigModal.getByTestId("llm-provider"); + await expect(providerSelectElement).toHaveValue("Anthropic"); + + const modelSelectElement = aiConfigModal.getByTestId("llm-model"); + await expect(modelSelectElement).toHaveValue(/claude-3.5/i); +}); diff --git a/openhands/server/routes/settings.py b/openhands/server/routes/settings.py index 9cb7d0105c..456bb2a873 100644 --- a/openhands/server/routes/settings.py +++ b/openhands/server/routes/settings.py @@ -12,7 +12,8 @@ app = APIRouter(prefix='/api') SettingsStoreImpl = get_impl(SettingsStore, openhands_config.settings_store_class) # type: ignore ConversationStoreImpl = get_impl( - ConversationStore, openhands_config.conversation_store_class # type: ignore + ConversationStore, # type: ignore + openhands_config.conversation_store_class, ) @@ -28,7 +29,7 @@ async def load_settings( settings = await settings_store.load() if settings: # For security reasons we don't ever send the api key to the client - settings.llm_api_key = None + settings.llm_api_key = 'SET' if settings.llm_api_key else None return settings except Exception as e: logger.warning(f'Invalid token: {e}') @@ -50,7 +51,10 @@ async def store_settings( settings_store = await SettingsStoreImpl.get_instance(config, github_token) existing_settings = await settings_store.load() if existing_settings: - settings = Settings(**{**existing_settings.__dict__, **settings.__dict__}) + # Only update settings that are not None with the new values + for key, value in settings.__dict__.items(): + if value is None: + setattr(settings, key, getattr(existing_settings, key)) if settings.llm_api_key is None: settings.llm_api_key = existing_settings.llm_api_key await settings_store.store(settings)