github/OpenHands
1
0
Fork 0
mirror of https://github.com/OpenHands/OpenHands.git synced 2025-12-26 05:48:36 +08:00
Code Issues Packages Projects Releases Wiki Activity

[Refactor]: Create dedicated reset settings route (#7472)

Browse Source 
Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
Rohit Malhotra 2025-03-25 18:34:55 -04:00 committed by GitHub
parent efcf30a23d
commit 5a3eca2a2a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 193 additions and 191 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

84
frontend/__tests__/routes/settings.test.tsx
View File

@ -8,7 +8,6 @@ import { AuthProvider } from "#/context/auth-context";
import SettingsScreen from "#/routes/settings";
import * as AdvancedSettingsUtlls from "#/utils/has-advanced-settings-set";
import { MOCK_DEFAULT_USER_SETTINGS } from "#/mocks/handlers";
import { PostApiSettings } from "#/types/settings";
import * as ConsentHandlers from "#/utils/handle-capture-consent";
import AccountSettings from "#/routes/account-settings";
@ -20,6 +19,7 @@ const toggleAdvancedSettings = async (user: UserEvent) => {
describe("Settings Screen", () => {
const getSettingsSpy = vi.spyOn(OpenHands, "getSettings");
const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings");
const resetSettingsSpy = vi.spyOn(OpenHands, "resetSettings");
const getConfigSpy = vi.spyOn(OpenHands, "getConfig");
const { handleLogoutMock } = vi.hoisted(() => ({
@ -583,6 +583,11 @@ describe("Settings Screen", () => {
test("resetting settings with no changes but having advanced enabled should hide the advanced items", async () => {
const user = userEvent.setup();
getSettingsSpy.mockResolvedValueOnce({
...MOCK_DEFAULT_USER_SETTINGS,
});
renderSettingsScreen();
await toggleAdvancedSettings(user);
@ -594,6 +599,15 @@ describe("Settings Screen", () => {
const modal = await screen.findByTestId("reset-modal");
expect(modal).toBeInTheDocument();
// Mock the settings that will be returned after reset
// This should be the default settings with no advanced settings enabled
getSettingsSpy.mockResolvedValueOnce({
...MOCK_DEFAULT_USER_SETTINGS,
llm_base_url: "",
confirmation_mode: false,
security_analyzer: "",
});
// confirm reset
const confirmButton = within(modal).getByText("Reset");
await user.click(confirmButton);
@ -817,19 +831,27 @@ describe("Settings Screen", () => {
const confirmButton = within(modal).getByText("Reset");
await user.click(confirmButton);
const mockCopy: Partial<PostApiSettings> = {
...MOCK_DEFAULT_USER_SETTINGS,
};
delete mockCopy.github_token_is_set;
delete mockCopy.unset_github_token;
delete mockCopy.user_consents_to_analytics;
expect(saveSettingsSpy).toHaveBeenCalledWith({
...mockCopy,
provider_tokens: undefined, // not set
llm_api_key: "", // reset as well
await waitFor(() => {
expect(resetSettingsSpy).toHaveBeenCalled();
});
// Mock the settings response after reset
getSettingsSpy.mockResolvedValueOnce({
...MOCK_DEFAULT_USER_SETTINGS,
llm_base_url: "",
confirmation_mode: false,
security_analyzer: "",
});
// Wait for the mutation to complete and the modal to be removed
await waitFor(() => {
expect(screen.queryByTestId("reset-modal")).not.toBeInTheDocument();
expect(screen.queryByTestId("llm-custom-model-input")).not.toBeInTheDocument();
expect(screen.queryByTestId("base-url-input")).not.toBeInTheDocument();
expect(screen.queryByTestId("agent-input")).not.toBeInTheDocument();
expect(screen.queryByTestId("security-analyzer-input")).not.toBeInTheDocument();
expect(screen.queryByTestId("enable-confirmation-mode-switch")).not.toBeInTheDocument();
});
expect(screen.queryByTestId("reset-modal")).not.toBeInTheDocument();
});
it("should cancel the reset when the 'Cancel' button is clicked", async () => {
@ -887,32 +909,6 @@ describe("Settings Screen", () => {
expect(handleCaptureConsentSpy).toHaveBeenCalledWith(false);
});
it("should not reset analytics consent when resetting to defaults", async () => {
const user = userEvent.setup();
getSettingsSpy.mockResolvedValue({
...MOCK_DEFAULT_USER_SETTINGS,
user_consents_to_analytics: true,
});
renderSettingsScreen();
const analyticsConsentInput = await screen.findByTestId(
"enable-analytics-switch",
);
expect(analyticsConsentInput).toBeChecked();
const resetButton = await screen.findByText("Reset to defaults");
await user.click(resetButton);
const modal = await screen.findByTestId("reset-modal");
const confirmButton = within(modal).getByText("Reset");
await user.click(confirmButton);
expect(saveSettingsSpy).toHaveBeenCalledWith(
expect.objectContaining({ user_consents_to_analytics: undefined }),
);
});
it("should render the security analyzer input if the confirmation mode is enabled", async () => {
const user = userEvent.setup();
renderSettingsScreen();
@ -1094,14 +1090,8 @@ describe("Settings Screen", () => {
const modal = await screen.findByTestId("reset-modal");
const confirmButton = within(modal).getByText("Reset");
await user.click(confirmButton);
expect(saveSettingsSpy).toHaveBeenCalledWith(
expect.objectContaining({
llm_api_key: undefined,
llm_base_url: undefined,
llm_model: undefined,
}),
);
expect(saveSettingsSpy).not.toHaveBeenCalled();
expect(resetSettingsSpy).toHaveBeenCalled();
});
});
});

14
frontend/src/api/open-hands.ts
View File

@ -273,6 +273,14 @@ class OpenHands {
return data.status === 200;
}
/**
* Reset user settings in server
*/
static async resetSettings(): Promise<boolean> {
const response = await openHands.post("/api/reset-settings");
return response.status === 200;
}
static async createCheckoutSession(amount: number): Promise<string> {
const { data } = await openHands.post(
"/api/billing/create-checkout-session",
@ -345,8 +353,10 @@ class OpenHands {
return data;
}
static async logout(): Promise<void> {
await openHands.post("/api/logout");
static async logout(appMode: GetConfigResponse["APP_MODE"]): Promise<void> {
const endpoint =
appMode === "saas" ? "/api/logout" : "/api/unset-settings-tokens";
await openHands.post(endpoint);
}
}

5
frontend/src/components/features/sidebar/sidebar.tsx
View File

@ -21,7 +21,6 @@ import { useLogout } from "#/hooks/mutation/use-logout";
import { useConfig } from "#/hooks/query/use-config";
import { cn } from "#/utils/utils";
import { displayErrorToast } from "#/utils/custom-toast-handlers";
import { useSaveSettings } from "#/hooks/mutation/use-save-settings";
export function Sidebar() {
const location = useLocation();
@ -36,7 +35,6 @@ export function Sidebar() {
isFetching: isFetchingSettings,
} = useSettings();
const { mutateAsync: logout } = useLogout();
const { mutate: saveUserSettings } = useSaveSettings();
const [settingsModalIsOpen, setSettingsModalIsOpen] = React.useState(false);
@ -78,8 +76,7 @@ export function Sidebar() {
};
const handleLogout = async () => {
if (config?.APP_MODE === "saas") await logout();
else saveUserSettings({ unset_github_token: true });
await logout();
posthog.reset();
};

16
frontend/src/hooks/mutation/use-logout.ts
View File

@ -1,16 +1,26 @@
import { useMutation, useQueryClient } from "@tanstack/react-query";
import OpenHands from "#/api/open-hands";
import { useAuth } from "#/context/auth-context";
import { useConfig } from "../query/use-config";
export const useLogout = () => {
const { setGitHubTokenIsSet } = useAuth();
const queryClient = useQueryClient();
const { data: config } = useConfig();
return useMutation({
mutationFn: OpenHands.logout,
onSuccess: async () => {
mutationFn: async () => {
// Pause all queries that depend on githubTokenIsSet
queryClient.setQueryData(["user"], null);
// Call logout endpoint
await OpenHands.logout(config?.APP_MODE ?? "oss");
// Remove settings from cache so it will be refetched with new token state
queryClient.removeQueries({ queryKey: ["settings"] });
// Update token state - this will trigger a settings refetch since it's part of the query key
setGitHubTokenIsSet(false);
await queryClient.invalidateQueries();
},
});
};

35
frontend/src/hooks/mutation/use-save-settings.ts
View File

@ -4,8 +4,14 @@ import OpenHands from "#/api/open-hands";
import { PostSettings, PostApiSettings } from "#/types/settings";
import { useSettings } from "../query/use-settings";
const saveSettingsMutationFn = async (settings: Partial<PostSettings>) => {
const resetLlmApiKey = settings.LLM_API_KEY === "";
const saveSettingsMutationFn = async (
settings: Partial<PostSettings> | null,
) => {
// If settings is null, we're resetting
if (settings === null) {
await OpenHands.resetSettings();
return;
}
const apiSettings: Partial<PostApiSettings> = {
llm_model: settings.LLM_MODEL,
@ -14,12 +20,12 @@ const saveSettingsMutationFn = async (settings: Partial<PostSettings>) => {
language: settings.LANGUAGE || DEFAULT_SETTINGS.LANGUAGE,
confirmation_mode: settings.CONFIRMATION_MODE,
security_analyzer: settings.SECURITY_ANALYZER,
llm_api_key: resetLlmApiKey
? ""
: settings.LLM_API_KEY?.trim() || undefined,
llm_api_key:
settings.LLM_API_KEY === ""
? ""
: settings.LLM_API_KEY?.trim() || undefined,
remote_runtime_resource_factor: settings.REMOTE_RUNTIME_RESOURCE_FACTOR,
provider_tokens: settings.provider_tokens,
unset_github_token: settings.unset_github_token,
enable_default_condenser: settings.ENABLE_DEFAULT_CONDENSER,
enable_sound_notifications: settings.ENABLE_SOUND_NOTIFICATIONS,
user_consents_to_analytics: settings.user_consents_to_analytics,
@ -33,20 +39,13 @@ export const useSaveSettings = () => {
const { data: currentSettings } = useSettings();
return useMutation({
mutationFn: async (settings: Partial<PostSettings>) => {
const newSettings = { ...currentSettings, ...settings };
// Temp hack for reset logic
if (
settings.LLM_API_KEY === undefined &&
settings.LLM_BASE_URL === undefined &&
settings.LLM_MODEL === undefined
) {
delete newSettings.LLM_API_KEY;
delete newSettings.LLM_BASE_URL;
delete newSettings.LLM_MODEL;
mutationFn: async (settings: Partial<PostSettings> | null) => {
if (settings === null) {
await saveSettingsMutationFn(null);
return;
}
const newSettings = { ...currentSettings, ...settings };
await saveSettingsMutationFn(newSettings);
},
onSuccess: async () => {

10
frontend/src/hooks/query/use-github-user.ts
View File

@ -5,13 +5,11 @@ import { useConfig } from "./use-config";
import OpenHands from "#/api/open-hands";
import { useAuth } from "#/context/auth-context";
import { useLogout } from "../mutation/use-logout";
import { useSaveSettings } from "../mutation/use-save-settings";
export const useGitHubUser = () => {
const { githubTokenIsSet } = useAuth();
const { setGitHubTokenIsSet } = useAuth();
const { mutateAsync: logout } = useLogout();
const { mutate: saveUserSettings } = useSaveSettings();
const { data: config } = useConfig();
const user = useQuery({
@ -36,11 +34,7 @@ export const useGitHubUser = () => {
}, [user.data]);
const handleLogout = async () => {
if (config?.APP_MODE === "saas") await logout();
else {
saveUserSettings({ unset_github_token: true });
setGitHubTokenIsSet(false);
}
await logout();
posthog.reset();
};

7
frontend/src/hooks/use-app-logout.ts
View File

@ -1,15 +1,10 @@
import { useLogout } from "./mutation/use-logout";
import { useSaveSettings } from "./mutation/use-save-settings";
import { useConfig } from "./query/use-config";
export const useAppLogout = () => {
const { data: config } = useConfig();
const { mutateAsync: logout } = useLogout();
const { mutate: saveUserSettings } = useSaveSettings();
const handleLogout = async () => {
if (config?.APP_MODE === "saas") await logout();
else saveUserSettings({ unset_github_token: true });
await logout();
};
return { handleLogout };

11
frontend/src/mocks/handlers.ts
View File

@ -206,11 +206,6 @@ export const handlers = [
let newSettings: Partial<PostApiSettings> = {};
if (typeof body === "object") {
newSettings = { ...body };
if (newSettings.unset_github_token) {
newSettings.provider_tokens = { github: "", gitlab: "" };
newSettings.github_token_is_set = false;
delete newSettings.unset_github_token;
}
}
const fullSettings = {
@ -306,4 +301,10 @@ export const handlers = [
}),
http.post("/api/logout", () => HttpResponse.json(null, { status: 200 })),
http.post("/api/reset-settings", async () => {
await delay();
MOCK_USER_PREFERENCES.settings = { ...MOCK_DEFAULT_USER_SETTINGS };
return HttpResponse.json(null, { status: 200 });
}),
];

18
frontend/src/routes/account-settings.tsx
View File

@ -25,7 +25,6 @@ import {
displayErrorToast,
displaySuccessToast,
} from "#/utils/custom-toast-handlers";
import { PostSettings } from "#/types/settings";
const REMOTE_RUNTIME_OPTIONS = [
{ key: 1, label: "1x (2 core, 8G)" },
@ -170,24 +169,11 @@ function AccountSettings() {
};
const handleReset = () => {
const newSettings: Partial<PostSettings> = {
...DEFAULT_SETTINGS,
LLM_API_KEY: "", // reset LLM API key
};
// we don't want the user to be able to modify these settings in SaaS
// and we should make sure they aren't included in the reset
if (shouldHandleSpecialSaasCase) {
delete newSettings.LLM_API_KEY;
delete newSettings.LLM_BASE_URL;
delete newSettings.LLM_MODEL;
}
saveSettings(newSettings, {
saveSettings(null, {
onSuccess: () => {
displaySuccessToast("Settings reset");
setResetSettingsModalIsOpen(false);
setLlmConfigMode(isAdvancedSettingsSet ? "advanced" : "basic");
setLlmConfigMode("basic");
},
});
};

2
frontend/src/types/settings.ts
View File

@ -35,12 +35,10 @@ export type ApiSettings = {
export type PostSettings = Settings & {
provider_tokens: Record<Provider, string>;
unset_github_token: boolean;
user_consents_to_analytics: boolean | null;
};
export type PostApiSettings = ApiSettings & {
provider_tokens: Record<Provider, string>;
unset_github_token: boolean;
user_consents_to_analytics: boolean | null;
};

134
openhands/server/routes/settings.py
View File

@ -7,7 +7,8 @@ from openhands.integrations.provider import ProviderToken, ProviderType, SecretS
from openhands.integrations.utils import validate_provider_token
from openhands.server.auth import get_provider_tokens, get_user_id
from openhands.server.settings import GETSettingsModel, POSTSettingsModel, Settings
from openhands.server.shared import SettingsStoreImpl, config
from openhands.server.shared import SettingsStoreImpl, config, server_config
from openhands.server.types import AppMode
app = APIRouter(prefix='/api')
@ -40,6 +41,79 @@ async def load_settings(request: Request) -> GETSettingsModel | JSONResponse:
)
@app.post('/unset-settings-tokens', response_model=dict[str, str])
async def unset_settings_tokens(
request: Request
) -> JSONResponse:
try:
settings_store = await SettingsStoreImpl.get_instance(
config, get_user_id(request)
)
existing_settings = await settings_store.load()
if existing_settings:
settings = existing_settings.model_copy(update={'secrets_store': SecretStore()})
await settings_store.store(settings)
return JSONResponse(
status_code=status.HTTP_200_OK,
content={'message': 'Settings stored'},
)
except Exception as e:
logger.warning(f'Something went wrong unsetting tokens: {e}')
return JSONResponse(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
content={'error': 'Something went wrong unsetting tokens'},
)
@app.post('/reset-settings', response_model=dict[str, str])
async def reset_settings(
request: Request
) -> JSONResponse:
"""
Resets user settings.
"""
try:
settings_store = await SettingsStoreImpl.get_instance(
config, get_user_id(request)
)
existing_settings = await settings_store.load()
settings = Settings(language="en",
agent="CodeActAgent",
security_analyzer="",
confirmation_mode=False,
llm_model="anthropic/claude-3-5-sonnet-20241022",
llm_api_key="",
llm_base_url="",
remote_runtime_resource_factor=1,
enable_default_condenser=True,
enable_sound_notifications=False,
user_consents_to_analytics=existing_settings.user_consents_to_analytics if existing_settings else False
)
server_config_values = server_config.get_config()
is_hide_llm_settings_enabled = server_config_values.get("FEATURE_FLAGS", {}).get("HIDE_LLM_SETTINGS", False)
# We don't want the user to be able to modify these settings in SaaS
if (server_config.app_mode == AppMode.SAAS and is_hide_llm_settings_enabled):
if existing_settings:
settings.llm_api_key = existing_settings.llm_api_key
settings.llm_base_url = existing_settings.llm_base_url
settings.llm_model = existing_settings.llm_model
await settings_store.store(settings)
return JSONResponse(
status_code=status.HTTP_200_OK,
content={'message': 'Settings stored'},
)
except Exception as e:
logger.warning(f'Something went wrong resetting settings: {e}')
return JSONResponse(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
content={'error': 'Something went wrong resetting settings'},
)
@app.post('/settings', response_model=dict[str, str])
async def store_settings(
request: Request,
@ -89,39 +163,35 @@ async def store_settings(
existing_settings.user_consents_to_analytics
)
# Handle token updates immutably
if settings.unset_github_token:
settings = settings.model_copy(update={'secrets_store': SecretStore()})
# Only merge if not unsetting tokens
if settings.provider_tokens:
if existing_settings.secrets_store:
existing_providers = [
provider.value
for provider in existing_settings.secrets_store.provider_tokens
]
else: # Only merge if not unsetting tokens
if settings.provider_tokens:
if existing_settings.secrets_store:
existing_providers = [
provider.value
for provider in existing_settings.secrets_store.provider_tokens
]
# Merge incoming settings store with the existing one
for provider, token_value in settings.provider_tokens.items():
if provider in existing_providers and not token_value:
provider_type = ProviderType(provider)
existing_token = (
existing_settings.secrets_store.provider_tokens.get(
provider_type
)
# Merge incoming settings store with the existing one
for provider, token_value in settings.provider_tokens.items():
if provider in existing_providers and not token_value:
provider_type = ProviderType(provider)
existing_token = (
existing_settings.secrets_store.provider_tokens.get(
provider_type
)
if existing_token and existing_token.token:
settings.provider_tokens[provider] = (
existing_token.token.get_secret_value()
)
else: # nothing passed in means keep current settings
provider_tokens = existing_settings.secrets_store.provider_tokens
settings.provider_tokens = {
provider.value: data.token.get_secret_value()
if data.token
else None
for provider, data in provider_tokens.items()
}
)
if existing_token and existing_token.token:
settings.provider_tokens[provider] = (
existing_token.token.get_secret_value()
)
else: # nothing passed in means keep current settings
provider_tokens = existing_settings.secrets_store.provider_tokens
settings.provider_tokens = {
provider.value: data.token.get_secret_value()
if data.token
else None
for provider, data in provider_tokens.items()
}
# Update sandbox config with new settings
if settings.remote_runtime_resource_factor is not None:

1
openhands/server/settings.py
View File

@ -107,7 +107,6 @@ class POSTSettingsModel(Settings):
Settings for POST requests
"""
unset_github_token: bool | None = None
# Override provider_tokens to accept string tokens from frontend
provider_tokens: dict[str, str] = {}

47
tests/unit/test_settings_api.py
View File

@ -209,53 +209,6 @@ async def test_settings_api_set_github_token(
assert data['token_is_set'] is True
@pytest.mark.skip(
reason='Mock middleware does not seem to properly set the github_token'
)
async def test_settings_unset_github_token(
mock_github_service,
test_client,
mock_settings_store,
mock_get_user_id,
mock_validate_provider_token,
):
# Test data with unset_token set to True
settings_data = {
'language': 'en',
'agent': 'test-agent',
'max_iterations': 100,
'security_analyzer': 'default',
'confirmation_mode': True,
'llm_model': 'test-model',
'llm_api_key': 'test-key',
'llm_base_url': 'https://test.com',
'provider_tokens': {'github': 'test-token'},
}
# Mock settings store to return our settings for the GET request
mock_settings_store.load.return_value = Settings(**settings_data)
response = test_client.get('/api/settings')
assert response.status_code == 200
assert response.json()['token_is_set'] is True
settings_data['unset_token'] = True
# Make the POST request to store settings
response = test_client.post('/api/settings', json=settings_data)
assert response.status_code == 200
# Verify the settings were stored with the provider token unset
stored_settings = mock_settings_store.store.call_args[0][0]
assert not stored_settings.secrets_store.provider_tokens
mock_settings_store.load.return_value = Settings(**stored_settings.dict())
# Make a GET request to retrieve settings
response = test_client.get('/api/settings')
assert response.status_code == 200
assert response.json()['token_is_set'] is False
@pytest.mark.asyncio
async def test_settings_preserve_llm_fields_when_none(test_client, mock_settings_store):
# Setup initial settings with LLM fields populated