From 0e20fc206b8aefe9ac4658b1f912459574828d4d Mon Sep 17 00:00:00 2001 From: Hiep Le <69354317+hieptl@users.noreply.github.com> Date: Thu, 11 Sep 2025 23:39:23 +0700 Subject: [PATCH] refactor(frontend): move settings APIs to a dedicated service handler (#10941) --- .../analytics-consent-form-modal.test.tsx | 4 +- .../features/home/repo-connector.test.tsx | 53 +++++++++++------ .../features/sidebar/sidebar.test.tsx | 4 +- .../modals/settings/settings-form.test.tsx | 4 +- .../hooks/mutation/use-save-settings.test.tsx | 4 +- frontend/__tests__/routes/_oh.test.tsx | 5 +- .../__tests__/routes/app-settings.test.tsx | 26 ++++---- .../__tests__/routes/git-settings.test.tsx | 11 ++-- .../__tests__/routes/home-screen.test.tsx | 7 ++- .../__tests__/routes/llm-settings.test.tsx | 59 ++++++++++++------- .../routes/secrets-settings.test.tsx | 12 +++- frontend/src/api/open-hands.ts | 21 +------ .../src/hooks/mutation/use-add-mcp-server.ts | 4 +- .../hooks/mutation/use-delete-mcp-server.ts | 4 +- .../src/hooks/mutation/use-save-settings.ts | 7 ++- .../hooks/mutation/use-update-mcp-server.ts | 4 +- frontend/src/hooks/query/use-settings.ts | 4 +- frontend/src/mocks/handlers.ts | 6 +- .../settings-service/settings-service.api.ts | 28 +++++++++ .../src/settings-service/settings.types.ts | 53 +++++++++++++++++ frontend/src/types/settings.ts | 38 ------------ 21 files changed, 216 insertions(+), 142 deletions(-) create mode 100644 frontend/src/settings-service/settings-service.api.ts create mode 100644 frontend/src/settings-service/settings.types.ts diff --git a/frontend/__tests__/components/features/analytics/analytics-consent-form-modal.test.tsx b/frontend/__tests__/components/features/analytics/analytics-consent-form-modal.test.tsx index 7400babc53..b5746d6d25 100644 --- a/frontend/__tests__/components/features/analytics/analytics-consent-form-modal.test.tsx +++ b/frontend/__tests__/components/features/analytics/analytics-consent-form-modal.test.tsx @@ -3,13 +3,13 @@ import { describe, expect, it, vi } from "vitest"; import { render, screen, waitFor } from "@testing-library/react"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { AnalyticsConsentFormModal } from "#/components/features/analytics/analytics-consent-form-modal"; -import OpenHands from "#/api/open-hands"; +import SettingsService from "#/settings-service/settings-service.api"; describe("AnalyticsConsentFormModal", () => { it("should call saveUserSettings with consent", async () => { const user = userEvent.setup(); const onCloseMock = vi.fn(); - const saveUserSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + const saveUserSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); render(, { wrapper: ({ children }) => ( diff --git a/frontend/__tests__/components/features/home/repo-connector.test.tsx b/frontend/__tests__/components/features/home/repo-connector.test.tsx index 43b1bffd35..cf83d6337d 100644 --- a/frontend/__tests__/components/features/home/repo-connector.test.tsx +++ b/frontend/__tests__/components/features/home/repo-connector.test.tsx @@ -5,6 +5,7 @@ import { QueryClientProvider, QueryClient } from "@tanstack/react-query"; import { setupStore } from "test-utils"; import { Provider } from "react-redux"; import { createRoutesStub, Outlet } from "react-router"; +import SettingsService from "#/settings-service/settings-service.api"; import OpenHands from "#/api/open-hands"; import { GitRepository } from "#/types/git"; import { RepoConnector } from "#/components/features/home/repo-connector"; @@ -66,7 +67,7 @@ const MOCK_RESPOSITORIES: GitRepository[] = [ ]; beforeEach(() => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, provider_tokens_set: { @@ -135,10 +136,16 @@ describe("RepoConnector", () => { expect(launchButton).toBeDisabled(); // Mock the repository branches API call - vi.spyOn(OpenHands, "getRepositoryBranches").mockResolvedValue({ branches: [ - { name: "main", commit_sha: "123", protected: false }, - { name: "develop", commit_sha: "456", protected: false }, - ], has_next_page: false, current_page: 1, per_page: 30, total_count: 2 }); + vi.spyOn(OpenHands, "getRepositoryBranches").mockResolvedValue({ + branches: [ + { name: "main", commit_sha: "123", protected: false }, + { name: "develop", commit_sha: "456", protected: false }, + ], + has_next_page: false, + current_page: 1, + per_page: 30, + total_count: 2, + }); // First select the provider const providerDropdown = await waitFor(() => @@ -177,7 +184,7 @@ describe("RepoConnector", () => { APP_SLUG: "openhands", }); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, provider_tokens_set: { @@ -224,7 +231,7 @@ describe("RepoConnector", () => { APP_SLUG: "openhands", }); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, provider_tokens_set: { @@ -268,7 +275,7 @@ describe("RepoConnector", () => { APP_MODE: "oss", }); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, provider_tokens_set: { @@ -340,10 +347,16 @@ describe("RepoConnector", () => { expect(createConversationSpy).not.toHaveBeenCalled(); // Mock the repository branches API call - vi.spyOn(OpenHands, "getRepositoryBranches").mockResolvedValue({ branches: [ - { name: "main", commit_sha: "123", protected: false }, - { name: "develop", commit_sha: "456", protected: false }, - ], has_next_page: false, current_page: 1, per_page: 30, total_count: 2 }); + vi.spyOn(OpenHands, "getRepositoryBranches").mockResolvedValue({ + branches: [ + { name: "main", commit_sha: "123", protected: false }, + { name: "develop", commit_sha: "456", protected: false }, + ], + has_next_page: false, + current_page: 1, + per_page: 30, + total_count: 2, + }); // First select the provider const providerDropdown = await waitFor(() => @@ -397,10 +410,16 @@ describe("RepoConnector", () => { }); // Mock the repository branches API call - vi.spyOn(OpenHands, "getRepositoryBranches").mockResolvedValue({ branches: [ - { name: "main", commit_sha: "123", protected: false }, - { name: "develop", commit_sha: "456", protected: false }, - ], has_next_page: false, current_page: 1, per_page: 30, total_count: 2 }); + vi.spyOn(OpenHands, "getRepositoryBranches").mockResolvedValue({ + branches: [ + { name: "main", commit_sha: "123", protected: false }, + { name: "develop", commit_sha: "456", protected: false }, + ], + has_next_page: false, + current_page: 1, + per_page: 30, + total_count: 2, + }); renderRepoConnector(); @@ -448,7 +467,7 @@ describe("RepoConnector", () => { }); it("should display a button to settings if the user needs to sign in with their git provider", async () => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, provider_tokens_set: {}, diff --git a/frontend/__tests__/components/features/sidebar/sidebar.test.tsx b/frontend/__tests__/components/features/sidebar/sidebar.test.tsx index 9229cb3828..4844a778e1 100644 --- a/frontend/__tests__/components/features/sidebar/sidebar.test.tsx +++ b/frontend/__tests__/components/features/sidebar/sidebar.test.tsx @@ -3,7 +3,7 @@ import { renderWithProviders } from "test-utils"; import { createRoutesStub } from "react-router"; import { waitFor } from "@testing-library/react"; import { Sidebar } from "#/components/features/sidebar/sidebar"; -import OpenHands from "#/api/open-hands"; +import SettingsService from "#/settings-service/settings-service.api"; // These tests will now fail because the conversation panel is rendered through a portal // and technically not a child of the Sidebar component. @@ -19,7 +19,7 @@ const renderSidebar = () => renderWithProviders(); describe("Sidebar", () => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); afterEach(() => { vi.clearAllMocks(); diff --git a/frontend/__tests__/components/shared/modals/settings/settings-form.test.tsx b/frontend/__tests__/components/shared/modals/settings/settings-form.test.tsx index d1d623f137..6b4616ab12 100644 --- a/frontend/__tests__/components/shared/modals/settings/settings-form.test.tsx +++ b/frontend/__tests__/components/shared/modals/settings/settings-form.test.tsx @@ -3,13 +3,13 @@ import { describe, expect, it, vi } from "vitest"; import { renderWithProviders } from "test-utils"; import { createRoutesStub } from "react-router"; import { screen } from "@testing-library/react"; -import OpenHands from "#/api/open-hands"; +import SettingsService from "#/settings-service/settings-service.api"; import { SettingsForm } from "#/components/shared/modals/settings/settings-form"; import { DEFAULT_SETTINGS } from "#/services/settings"; describe("SettingsForm", () => { const onCloseMock = vi.fn(); - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); const RouteStub = createRoutesStub([ { diff --git a/frontend/__tests__/hooks/mutation/use-save-settings.test.tsx b/frontend/__tests__/hooks/mutation/use-save-settings.test.tsx index 37c8695096..29fdb99273 100644 --- a/frontend/__tests__/hooks/mutation/use-save-settings.test.tsx +++ b/frontend/__tests__/hooks/mutation/use-save-settings.test.tsx @@ -1,12 +1,12 @@ import { renderHook, waitFor } from "@testing-library/react"; import { describe, expect, it, vi } from "vitest"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; -import OpenHands from "#/api/open-hands"; +import SettingsService from "#/settings-service/settings-service.api"; import { useSaveSettings } from "#/hooks/mutation/use-save-settings"; describe("useSaveSettings", () => { it("should send an empty string for llm_api_key if an empty string is passed, otherwise undefined", async () => { - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); const { result } = renderHook(() => useSaveSettings(), { wrapper: ({ children }) => ( diff --git a/frontend/__tests__/routes/_oh.test.tsx b/frontend/__tests__/routes/_oh.test.tsx index 2f1ac566e9..efffc54d58 100644 --- a/frontend/__tests__/routes/_oh.test.tsx +++ b/frontend/__tests__/routes/_oh.test.tsx @@ -9,6 +9,7 @@ import userEvent from "@testing-library/user-event"; import MainApp from "#/routes/root-layout"; import i18n from "#/i18n"; import * as CaptureConsent from "#/utils/handle-capture-consent"; +import SettingsService from "#/settings-service/settings-service.api"; import OpenHands from "#/api/open-hands"; import * as ToastHandlers from "#/utils/custom-toast-handlers"; @@ -63,7 +64,7 @@ describe("frontend/routes/_oh", () => { it.skip("should render and capture the user's consent if oss mode", async () => { const user = userEvent.setup(); const getConfigSpy = vi.spyOn(OpenHands, "getConfig"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); const handleCaptureConsentSpy = vi.spyOn( CaptureConsent, "handleCaptureConsent", @@ -185,7 +186,7 @@ describe("frontend/routes/_oh", () => { it("should render a you're in toast if it is a new user and in saas mode", async () => { const getConfigSpy = vi.spyOn(OpenHands, "getConfig"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); const displaySuccessToastSpy = vi.spyOn( ToastHandlers, "displaySuccessToast", diff --git a/frontend/__tests__/routes/app-settings.test.tsx b/frontend/__tests__/routes/app-settings.test.tsx index 4927846c3c..31b0f6b829 100644 --- a/frontend/__tests__/routes/app-settings.test.tsx +++ b/frontend/__tests__/routes/app-settings.test.tsx @@ -3,7 +3,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import userEvent from "@testing-library/user-event"; import AppSettingsScreen from "#/routes/app-settings"; -import OpenHands from "#/api/open-hands"; +import SettingsService from "#/settings-service/settings-service.api"; import { MOCK_DEFAULT_USER_SETTINGS } from "#/mocks/handlers"; import { AvailableLanguages } from "#/i18n"; import * as CaptureConsent from "#/utils/handle-capture-consent"; @@ -25,7 +25,7 @@ describe("Content", () => { }); it("should render the correct default values", async () => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, language: "no", @@ -65,8 +65,8 @@ describe("Form submission", () => { }); it("should submit the form with the correct values", async () => { - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue(MOCK_DEFAULT_USER_SETTINGS); renderAppSettingsScreen(); @@ -106,7 +106,7 @@ describe("Form submission", () => { }); it("should only enable the submit button when there are changes", async () => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue(MOCK_DEFAULT_USER_SETTINGS); renderAppSettingsScreen(); @@ -146,7 +146,7 @@ describe("Form submission", () => { }); it("should call handleCaptureConsents with true when the analytics switch is toggled", async () => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue(MOCK_DEFAULT_USER_SETTINGS); const handleCaptureConsentsSpy = vi.spyOn( @@ -168,7 +168,7 @@ describe("Form submission", () => { }); it("should call handleCaptureConsents with false when the analytics switch is toggled", async () => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, user_consents_to_analytics: true, @@ -215,8 +215,8 @@ describe("Form submission", () => { }); it("should disable the button after submitting changes", async () => { - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue(MOCK_DEFAULT_USER_SETTINGS); renderAppSettingsScreen(); @@ -240,8 +240,8 @@ describe("Form submission", () => { describe("Status toasts", () => { it("should call displaySuccessToast when the settings are saved", async () => { - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue(MOCK_DEFAULT_USER_SETTINGS); const displaySuccessToastSpy = vi.spyOn( @@ -265,8 +265,8 @@ describe("Status toasts", () => { }); it("should call displayErrorToast when the settings fail to save", async () => { - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue(MOCK_DEFAULT_USER_SETTINGS); const displayErrorToastSpy = vi.spyOn(ToastHandlers, "displayErrorToast"); diff --git a/frontend/__tests__/routes/git-settings.test.tsx b/frontend/__tests__/routes/git-settings.test.tsx index e09ad9cc7e..c1d63ded4d 100644 --- a/frontend/__tests__/routes/git-settings.test.tsx +++ b/frontend/__tests__/routes/git-settings.test.tsx @@ -6,6 +6,7 @@ import userEvent from "@testing-library/user-event"; import i18next from "i18next"; import { I18nextProvider } from "react-i18next"; import GitSettingsScreen from "#/routes/git-settings"; +import SettingsService from "#/settings-service/settings-service.api"; import OpenHands from "#/api/open-hands"; import AuthService from "#/api/auth-service/auth-service.api"; import { MOCK_DEFAULT_USER_SETTINGS } from "#/mocks/handlers"; @@ -153,7 +154,7 @@ describe("Content", () => { it("should set '' placeholder and indicator if the GitHub token is set", async () => { const getConfigSpy = vi.spyOn(OpenHands, "getConfig"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getConfigSpy.mockResolvedValue(VALID_OSS_CONFIG); getSettingsSpy.mockResolvedValue({ @@ -359,7 +360,7 @@ describe("Form submission", () => { it("should enable a disconnect tokens button if there is at least one token set", async () => { const getConfigSpy = vi.spyOn(OpenHands, "getConfig"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getConfigSpy.mockResolvedValue(VALID_OSS_CONFIG); getSettingsSpy.mockResolvedValue({ @@ -394,7 +395,7 @@ describe("Form submission", () => { it("should call logout when pressing the disconnect tokens button", async () => { const getConfigSpy = vi.spyOn(OpenHands, "getConfig"); const logoutSpy = vi.spyOn(AuthService, "logout"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getConfigSpy.mockResolvedValue(VALID_OSS_CONFIG); getSettingsSpy.mockResolvedValue({ @@ -477,7 +478,7 @@ describe("Form submission", () => { describe("Status toasts", () => { it("should call displaySuccessToast when the settings are saved", async () => { const saveProvidersSpy = vi.spyOn(SecretsService, "addGitProvider"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue(MOCK_DEFAULT_USER_SETTINGS); const displaySuccessToastSpy = vi.spyOn( @@ -500,7 +501,7 @@ describe("Status toasts", () => { it("should call displayErrorToast when the settings fail to save", async () => { const saveProvidersSpy = vi.spyOn(SecretsService, "addGitProvider"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue(MOCK_DEFAULT_USER_SETTINGS); const displayErrorToastSpy = vi.spyOn(ToastHandlers, "displayErrorToast"); diff --git a/frontend/__tests__/routes/home-screen.test.tsx b/frontend/__tests__/routes/home-screen.test.tsx index 7a004d3d57..5bfbb681ab 100644 --- a/frontend/__tests__/routes/home-screen.test.tsx +++ b/frontend/__tests__/routes/home-screen.test.tsx @@ -7,6 +7,7 @@ import { Provider } from "react-redux"; import { createAxiosNotFoundErrorObject, setupStore } from "test-utils"; import HomeScreen from "#/routes/home"; import { GitRepository } from "#/types/git"; +import SettingsService from "#/settings-service/settings-service.api"; import OpenHands from "#/api/open-hands"; import MainApp from "#/routes/root-layout"; import { MOCK_DEFAULT_USER_SETTINGS } from "#/mocks/handlers"; @@ -91,7 +92,7 @@ const MOCK_RESPOSITORIES: GitRepository[] = [ describe("HomeScreen", () => { beforeEach(() => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, provider_tokens_set: { @@ -359,7 +360,7 @@ describe("Settings 404", () => { }); const getConfigSpy = vi.spyOn(OpenHands, "getConfig"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); it("should open the settings modal if GET /settings fails with a 404", async () => { const error = createAxiosNotFoundErrorObject(); @@ -418,7 +419,7 @@ describe("Settings 404", () => { describe("Setup Payment modal", () => { const getConfigSpy = vi.spyOn(OpenHands, "getConfig"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); it("should only render if SaaS mode and is new user", async () => { // @ts-expect-error - we only need the APP_MODE for this test diff --git a/frontend/__tests__/routes/llm-settings.test.tsx b/frontend/__tests__/routes/llm-settings.test.tsx index aa3cabe2bd..3ee2b7c313 100644 --- a/frontend/__tests__/routes/llm-settings.test.tsx +++ b/frontend/__tests__/routes/llm-settings.test.tsx @@ -3,6 +3,7 @@ import userEvent from "@testing-library/user-event"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { QueryClientProvider, QueryClient } from "@tanstack/react-query"; import LlmSettingsScreen from "#/routes/llm-settings"; +import SettingsService from "#/settings-service/settings-service.api"; import OpenHands from "#/api/open-hands"; import { MOCK_DEFAULT_USER_SETTINGS, @@ -56,7 +57,7 @@ describe("Content", () => { }); it("should render the existing settings values", async () => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, llm_model: "openai/gpt-4o", @@ -84,7 +85,9 @@ describe("Content", () => { renderLlmSettingsScreen(); await screen.findByTestId("llm-settings-screen"); - const confirmation = screen.getByTestId("enable-confirmation-mode-switch"); + const confirmation = screen.getByTestId( + "enable-confirmation-mode-switch", + ); // Initially confirmation mode is false, so security analyzer should not be visible expect(confirmation).not.toBeChecked(); @@ -185,7 +188,7 @@ describe("Content", () => { }); it("should render existing advanced settings correctly", async () => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, llm_model: "openai/gpt-4o", @@ -230,7 +233,7 @@ describe("Content", () => { describe("Form submission", () => { it("should submit the basic form with the correct values", async () => { - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); renderLlmSettingsScreen(); await screen.findByTestId("llm-settings-screen"); @@ -266,7 +269,7 @@ describe("Form submission", () => { }); it("should submit the advanced form with the correct values", async () => { - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); renderLlmSettingsScreen(); await screen.findByTestId("llm-settings-screen"); @@ -310,7 +313,9 @@ describe("Form submission", () => { // select security analyzer const securityAnalyzer = screen.getByTestId("security-analyzer-input"); await userEvent.click(securityAnalyzer); - const securityAnalyzerOption = screen.getByText("SETTINGS$SECURITY_ANALYZER_NONE"); + const securityAnalyzerOption = screen.getByText( + "SETTINGS$SECURITY_ANALYZER_NONE", + ); await userEvent.click(securityAnalyzerOption); const submitButton = screen.getByTestId("submit-button"); @@ -329,7 +334,7 @@ describe("Form submission", () => { }); it("should disable the button if there are no changes in the basic form", async () => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, llm_model: "openai/gpt-4o", @@ -372,7 +377,7 @@ describe("Form submission", () => { }); it("should disable the button if there are no changes in the advanced form", async () => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, llm_model: "openai/gpt-4o", @@ -392,10 +397,14 @@ describe("Form submission", () => { const baseUrl = await screen.findByTestId("base-url-input"); const apiKey = await screen.findByTestId("llm-api-key-input"); const agent = await screen.findByTestId("agent-input"); - const condensor = await screen.findByTestId("enable-memory-condenser-switch"); + const condensor = await screen.findByTestId( + "enable-memory-condenser-switch", + ); // Confirmation mode switch is now in basic settings, always visible - const confirmation = await screen.findByTestId("enable-confirmation-mode-switch"); + const confirmation = await screen.findByTestId( + "enable-confirmation-mode-switch", + ); // enter custom model await userEvent.type(model, "-mini"); @@ -468,9 +477,13 @@ describe("Form submission", () => { expect(submitButton).toBeDisabled(); // select security analyzer - const securityAnalyzer = await screen.findByTestId("security-analyzer-input"); + const securityAnalyzer = await screen.findByTestId( + "security-analyzer-input", + ); await userEvent.click(securityAnalyzer); - const securityAnalyzerOption = screen.getByText("SETTINGS$SECURITY_ANALYZER_NONE"); + const securityAnalyzerOption = screen.getByText( + "SETTINGS$SECURITY_ANALYZER_NONE", + ); await userEvent.click(securityAnalyzerOption); expect(securityAnalyzer).toHaveValue("SETTINGS$SECURITY_ANALYZER_NONE"); @@ -478,9 +491,13 @@ describe("Form submission", () => { // revert back to original value await userEvent.click(securityAnalyzer); - const originalSecurityAnalyzerOption = screen.getByText("SETTINGS$SECURITY_ANALYZER_LLM_DEFAULT"); + const originalSecurityAnalyzerOption = screen.getByText( + "SETTINGS$SECURITY_ANALYZER_LLM_DEFAULT", + ); await userEvent.click(originalSecurityAnalyzerOption); - expect(securityAnalyzer).toHaveValue("SETTINGS$SECURITY_ANALYZER_LLM_DEFAULT"); + expect(securityAnalyzer).toHaveValue( + "SETTINGS$SECURITY_ANALYZER_LLM_DEFAULT", + ); expect(submitButton).toBeDisabled(); }); @@ -512,7 +529,7 @@ describe("Form submission", () => { // flaky test it.skip("should disable the button when submitting changes", async () => { - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); renderLlmSettingsScreen(); await screen.findByTestId("llm-settings-screen"); @@ -539,7 +556,7 @@ describe("Form submission", () => { }); it("should clear advanced settings when saving basic settings", async () => { - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); getSettingsSpy.mockResolvedValue({ ...MOCK_DEFAULT_USER_SETTINGS, llm_model: "openai/gpt-4o", @@ -547,7 +564,7 @@ describe("Form submission", () => { llm_api_key_set: true, confirmation_mode: true, }); - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); renderLlmSettingsScreen(); await screen.findByTestId("llm-settings-screen"); @@ -583,7 +600,7 @@ describe("Form submission", () => { describe("Status toasts", () => { describe("Basic form", () => { it("should call displaySuccessToast when the settings are saved", async () => { - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); const displaySuccessToastSpy = vi.spyOn( ToastHandlers, @@ -604,7 +621,7 @@ describe("Status toasts", () => { }); it("should call displayErrorToast when the settings fail to save", async () => { - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); const displayErrorToastSpy = vi.spyOn(ToastHandlers, "displayErrorToast"); @@ -626,7 +643,7 @@ describe("Status toasts", () => { describe("Advanced form", () => { it("should call displaySuccessToast when the settings are saved", async () => { - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); const displaySuccessToastSpy = vi.spyOn( ToastHandlers, @@ -652,7 +669,7 @@ describe("Status toasts", () => { }); it("should call displayErrorToast when the settings fail to save", async () => { - const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + const saveSettingsSpy = vi.spyOn(SettingsService, "saveSettings"); const displayErrorToastSpy = vi.spyOn(ToastHandlers, "displayErrorToast"); diff --git a/frontend/__tests__/routes/secrets-settings.test.tsx b/frontend/__tests__/routes/secrets-settings.test.tsx index 08c7d6f65b..aff96704b3 100644 --- a/frontend/__tests__/routes/secrets-settings.test.tsx +++ b/frontend/__tests__/routes/secrets-settings.test.tsx @@ -6,6 +6,7 @@ import { createRoutesStub, Outlet } from "react-router"; import SecretsSettingsScreen from "#/routes/secrets-settings"; import { SecretsService } from "#/api/secrets-service"; import { GetSecretsResponse } from "#/api/secrets-service.types"; +import SettingsService from "#/settings-service/settings-service.api"; import OpenHands from "#/api/open-hands"; import { MOCK_DEFAULT_USER_SETTINGS } from "#/mocks/handlers"; @@ -68,7 +69,7 @@ describe("Content", () => { it("should NOT render a button to connect with git if they havent already in oss", async () => { const getConfigSpy = vi.spyOn(OpenHands, "getConfig"); - const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); const getSecretsSpy = vi.spyOn(SecretsService, "getSecrets"); // @ts-expect-error - only return the config we need getConfigSpy.mockResolvedValue({ @@ -88,6 +89,7 @@ describe("Content", () => { it("should render add secret button in saas mode", async () => { const getConfigSpy = vi.spyOn(OpenHands, "getConfig"); + const getSettingsSpy = vi.spyOn(SettingsService, "getSettings"); const getSecretsSpy = vi.spyOn(SecretsService, "getSecrets"); // @ts-expect-error - only return the config we need getConfigSpy.mockResolvedValue({ @@ -476,7 +478,9 @@ describe("Secret actions", () => { // make POST request expect(createSecretSpy).not.toHaveBeenCalled(); - expect(screen.queryByText("SECRETS$SECRET_ALREADY_EXISTS")).toBeInTheDocument(); + expect( + screen.queryByText("SECRETS$SECRET_ALREADY_EXISTS"), + ).toBeInTheDocument(); await userEvent.clear(nameInput); await userEvent.type(nameInput, "My_Custom_Secret"); @@ -560,7 +564,9 @@ describe("Secret actions", () => { // make POST request expect(createSecretSpy).not.toHaveBeenCalled(); - expect(screen.queryByText("SECRETS$SECRET_ALREADY_EXISTS")).toBeInTheDocument(); + expect( + screen.queryByText("SECRETS$SECRET_ALREADY_EXISTS"), + ).toBeInTheDocument(); expect(nameInput).toHaveValue(MOCK_GET_SECRETS_RESPONSE[0].name); expect(valueInput).toHaveValue("my-custom-secret-value"); diff --git a/frontend/src/api/open-hands.ts b/frontend/src/api/open-hands.ts index dc009b7e9f..4512ba2144 100644 --- a/frontend/src/api/open-hands.ts +++ b/frontend/src/api/open-hands.ts @@ -18,7 +18,7 @@ import { GetFileResponse, } from "./open-hands.types"; import { openHands } from "./open-hands-axios"; -import { ApiSettings, PostApiSettings, Provider } from "#/types/settings"; +import { Provider } from "#/types/settings"; import { SuggestedTask } from "#/utils/types"; import { GitUser, @@ -359,25 +359,6 @@ 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; - } - static async createCheckoutSession(amount: number): Promise { const { data } = await openHands.post( "/api/billing/create-checkout-session", diff --git a/frontend/src/hooks/mutation/use-add-mcp-server.ts b/frontend/src/hooks/mutation/use-add-mcp-server.ts index f329adde7e..4c722cdd37 100644 --- a/frontend/src/hooks/mutation/use-add-mcp-server.ts +++ b/frontend/src/hooks/mutation/use-add-mcp-server.ts @@ -1,6 +1,6 @@ import { useMutation, useQueryClient } from "@tanstack/react-query"; import { useSettings } from "#/hooks/query/use-settings"; -import OpenHands from "#/api/open-hands"; +import SettingsService from "#/settings-service/settings-service.api"; import { MCPSSEServer, MCPStdioServer, MCPSHTTPServer } from "#/types/settings"; type MCPServerType = "sse" | "stdio" | "shttp"; @@ -57,7 +57,7 @@ export function useAddMcpServer() { mcp_config: newConfig, }; - await OpenHands.saveSettings(apiSettings); + await SettingsService.saveSettings(apiSettings); }, onSuccess: () => { // Invalidate the settings query to trigger a refetch diff --git a/frontend/src/hooks/mutation/use-delete-mcp-server.ts b/frontend/src/hooks/mutation/use-delete-mcp-server.ts index 6bcb9735f9..f060890ae8 100644 --- a/frontend/src/hooks/mutation/use-delete-mcp-server.ts +++ b/frontend/src/hooks/mutation/use-delete-mcp-server.ts @@ -1,6 +1,6 @@ import { useMutation, useQueryClient } from "@tanstack/react-query"; import { useSettings } from "#/hooks/query/use-settings"; -import OpenHands from "#/api/open-hands"; +import SettingsService from "#/settings-service/settings-service.api"; import { MCPConfig } from "#/types/settings"; export function useDeleteMcpServer() { @@ -27,7 +27,7 @@ export function useDeleteMcpServer() { mcp_config: newConfig, }; - await OpenHands.saveSettings(apiSettings); + await SettingsService.saveSettings(apiSettings); }, onSuccess: () => { // Invalidate the settings query to trigger a refetch diff --git a/frontend/src/hooks/mutation/use-save-settings.ts b/frontend/src/hooks/mutation/use-save-settings.ts index 68ecd6fb56..c0c6d5e09e 100644 --- a/frontend/src/hooks/mutation/use-save-settings.ts +++ b/frontend/src/hooks/mutation/use-save-settings.ts @@ -1,8 +1,9 @@ import { useMutation, useQueryClient } from "@tanstack/react-query"; import posthog from "posthog-js"; import { DEFAULT_SETTINGS } from "#/services/settings"; -import OpenHands from "#/api/open-hands"; -import { PostSettings, PostApiSettings } from "#/types/settings"; +import SettingsService from "#/settings-service/settings-service.api"; +import { PostSettings } from "#/types/settings"; +import { PostApiSettings } from "#/settings-service/settings.types"; import { useSettings } from "../query/use-settings"; const saveSettingsMutationFn = async (settings: Partial) => { @@ -36,7 +37,7 @@ const saveSettingsMutationFn = async (settings: Partial) => { settings.GIT_USER_EMAIL?.trim() || DEFAULT_SETTINGS.GIT_USER_EMAIL, }; - await OpenHands.saveSettings(apiSettings); + await SettingsService.saveSettings(apiSettings); }; export const useSaveSettings = () => { diff --git a/frontend/src/hooks/mutation/use-update-mcp-server.ts b/frontend/src/hooks/mutation/use-update-mcp-server.ts index 8505f93903..b727597baa 100644 --- a/frontend/src/hooks/mutation/use-update-mcp-server.ts +++ b/frontend/src/hooks/mutation/use-update-mcp-server.ts @@ -1,6 +1,6 @@ import { useMutation, useQueryClient } from "@tanstack/react-query"; import { useSettings } from "#/hooks/query/use-settings"; -import OpenHands from "#/api/open-hands"; +import SettingsService from "#/settings-service/settings-service.api"; import { MCPSSEServer, MCPStdioServer, MCPSHTTPServer } from "#/types/settings"; type MCPServerType = "sse" | "stdio" | "shttp"; @@ -59,7 +59,7 @@ export function useUpdateMcpServer() { mcp_config: newConfig, }; - await OpenHands.saveSettings(apiSettings); + await SettingsService.saveSettings(apiSettings); }, onSuccess: () => { // Invalidate the settings query to trigger a refetch diff --git a/frontend/src/hooks/query/use-settings.ts b/frontend/src/hooks/query/use-settings.ts index 65467ac46b..957f65aa6e 100644 --- a/frontend/src/hooks/query/use-settings.ts +++ b/frontend/src/hooks/query/use-settings.ts @@ -1,14 +1,14 @@ import { useQuery } from "@tanstack/react-query"; import React from "react"; import posthog from "posthog-js"; -import OpenHands from "#/api/open-hands"; +import SettingsService from "#/settings-service/settings-service.api"; import { DEFAULT_SETTINGS } from "#/services/settings"; import { useIsOnTosPage } from "#/hooks/use-is-on-tos-page"; import { Settings } from "#/types/settings"; import { useIsAuthed } from "./use-is-authed"; const getSettingsQueryFn = async (): Promise => { - const apiSettings = await OpenHands.getSettings(); + const apiSettings = await SettingsService.getSettings(); return { LLM_MODEL: apiSettings.llm_model, diff --git a/frontend/src/mocks/handlers.ts b/frontend/src/mocks/handlers.ts index 3b97b0d318..e22f53bd32 100644 --- a/frontend/src/mocks/handlers.ts +++ b/frontend/src/mocks/handlers.ts @@ -6,7 +6,11 @@ import { } from "#/api/open-hands.types"; import { DEFAULT_SETTINGS } from "#/services/settings"; import { STRIPE_BILLING_HANDLERS } from "./billing-handlers"; -import { ApiSettings, PostApiSettings, Provider } from "#/types/settings"; +import { Provider } from "#/types/settings"; +import { + ApiSettings, + PostApiSettings, +} from "#/settings-service/settings.types"; import { FILE_SERVICE_HANDLERS } from "./file-service-handlers"; import { GitUser } from "#/types/git"; import { TASK_SUGGESTIONS_HANDLERS } from "./task-suggestions-handlers"; diff --git a/frontend/src/settings-service/settings-service.api.ts b/frontend/src/settings-service/settings-service.api.ts new file mode 100644 index 0000000000..6d7309b8d1 --- /dev/null +++ b/frontend/src/settings-service/settings-service.api.ts @@ -0,0 +1,28 @@ +import { openHands } from "../api/open-hands-axios"; +import { ApiSettings, PostApiSettings } from "./settings.types"; + +/** + * Settings service for managing application settings + */ +class SettingsService { + /** + * 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 SettingsService; diff --git a/frontend/src/settings-service/settings.types.ts b/frontend/src/settings-service/settings.types.ts new file mode 100644 index 0000000000..bdd1610f49 --- /dev/null +++ b/frontend/src/settings-service/settings.types.ts @@ -0,0 +1,53 @@ +import { Provider } from "#/types/settings"; + +export type ApiSettings = { + llm_model: string; + llm_base_url: string; + agent: string; + language: string; + llm_api_key: string | null; + llm_api_key_set: boolean; + search_api_key_set: boolean; + confirmation_mode: boolean; + security_analyzer: string | null; + remote_runtime_resource_factor: number | null; + enable_default_condenser: boolean; + // Max size for condenser in backend settings + condenser_max_size: number | null; + enable_sound_notifications: boolean; + enable_proactive_conversation_starters: boolean; + enable_solvability_analysis: boolean; + user_consents_to_analytics: boolean | null; + search_api_key?: string; + provider_tokens_set: Partial>; + max_budget_per_task: number | null; + mcp_config?: { + sse_servers: (string | { url: string; api_key?: string })[]; + stdio_servers: { + name: string; + command: string; + args?: string[]; + env?: Record; + }[]; + shttp_servers: (string | { url: string; api_key?: string })[]; + }; + email?: string; + email_verified?: boolean; + git_user_name?: string; + git_user_email?: string; +}; + +export type PostApiSettings = ApiSettings & { + user_consents_to_analytics: boolean | null; + search_api_key?: string; + mcp_config?: { + sse_servers: (string | { url: string; api_key?: string })[]; + stdio_servers: { + name: string; + command: string; + args?: string[]; + env?: Record; + }[]; + shttp_servers: (string | { url: string; api_key?: string })[]; + }; +}; diff --git a/frontend/src/types/settings.ts b/frontend/src/types/settings.ts index 67460fec13..c82187389f 100644 --- a/frontend/src/types/settings.ts +++ b/frontend/src/types/settings.ts @@ -63,47 +63,9 @@ export type Settings = { GIT_USER_EMAIL?: string; }; -export type ApiSettings = { - llm_model: string; - llm_base_url: string; - agent: string; - language: string; - llm_api_key: string | null; - llm_api_key_set: boolean; - search_api_key_set: boolean; - confirmation_mode: boolean; - security_analyzer: string | null; - remote_runtime_resource_factor: number | null; - enable_default_condenser: boolean; - // Max size for condenser in backend settings - condenser_max_size: number | null; - enable_sound_notifications: boolean; - enable_proactive_conversation_starters: boolean; - enable_solvability_analysis: boolean; - user_consents_to_analytics: boolean | null; - search_api_key?: string; - provider_tokens_set: Partial>; - max_budget_per_task: number | null; - mcp_config?: { - sse_servers: (string | MCPSSEServer)[]; - stdio_servers: MCPStdioServer[]; - shttp_servers: (string | MCPSHTTPServer)[]; - }; - email?: string; - email_verified?: boolean; - git_user_name?: string; - git_user_email?: string; -}; - export type PostSettings = Settings & { user_consents_to_analytics: boolean | null; llm_api_key?: string | null; search_api_key?: string; mcp_config?: MCPConfig; }; - -export type PostApiSettings = ApiSettings & { - user_consents_to_analytics: boolean | null; - search_api_key?: string; - mcp_config?: MCPConfig; -};