From adff1c93d0a987b95eec0cf5ab28dad2fc76c324 Mon Sep 17 00:00:00 2001 From: Ryan Crabbe Date: Mon, 27 Apr 2026 14:27:11 -0700 Subject: [PATCH] refactor(ui): simplify LoggingSettings save flow via React Query callbacks Switch the spend-logs save flow from mutateAsync + try/catch to mutate + callbacks. Errors now surface through a single onError path (no more double toast on failure), and the delete-then-update sequencing runs through onSettled instead of awaited promises. handleFormSubmit is no longer async. Tighten the corresponding test to assert exactly one error toast fires. --- .../LoggingSettings/LoggingSettings.test.tsx | 79 +++++++------------ .../LoggingSettings/LoggingSettings.tsx | 67 ++++++++-------- 2 files changed, 61 insertions(+), 85 deletions(-) diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/LoggingSettings/LoggingSettings.test.tsx b/ui/litellm-dashboard/src/components/Settings/AdminSettings/LoggingSettings/LoggingSettings.test.tsx index 228e899a3a..74413333ec 100644 --- a/ui/litellm-dashboard/src/components/Settings/AdminSettings/LoggingSettings/LoggingSettings.test.tsx +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/LoggingSettings/LoggingSettings.test.tsx @@ -36,18 +36,18 @@ const mockNotificationsManager = vi.mocked(NotificationsManager); const mockParseErrorMessage = vi.mocked(parseErrorMessage); describe("LoggingSettings", () => { - const mockMutateAsync = vi.fn(); + const mockMutate = vi.fn(); const mockDeleteField = vi.fn(); const mockRefetch = vi.fn(); beforeEach(() => { vi.clearAllMocks(); mockUseStoreRequestInSpendLogs.mockReturnValue({ - mutateAsync: mockMutateAsync, + mutate: mockMutate, isPending: false, } as any); mockUseDeleteProxyConfigField.mockReturnValue({ - mutateAsync: mockDeleteField, + mutate: mockDeleteField, isPending: false, } as any); mockUseProxyConfig.mockReturnValue({ @@ -94,10 +94,8 @@ describe("LoggingSettings", () => { it("should submit form with store prompts enabled and retention period", async () => { const user = userEvent.setup(); - mockMutateAsync.mockImplementation(async (_params, options) => { - await Promise.resolve(); + mockMutate.mockImplementation((_params, options) => { options?.onSuccess?.(); - return { message: "Success" }; }); renderWithProviders(); @@ -113,7 +111,7 @@ describe("LoggingSettings", () => { await waitFor(() => { expect(mockDeleteField).not.toHaveBeenCalled(); - expect(mockMutateAsync).toHaveBeenCalledWith( + expect(mockMutate).toHaveBeenCalledWith( { store_prompts_in_spend_logs: true, maximum_spend_logs_retention_period: "30d", @@ -125,11 +123,11 @@ describe("LoggingSettings", () => { it("should delete retention period field when left empty on submit", async () => { const user = userEvent.setup(); - mockDeleteField.mockResolvedValue({ message: "Field deleted successfully" }); - mockMutateAsync.mockImplementation(async (_params, options) => { - await Promise.resolve(); + mockDeleteField.mockImplementation((_params, options) => { + options?.onSettled?.(); + }); + mockMutate.mockImplementation((_params, options) => { options?.onSuccess?.(); - return { message: "Success" }; }); renderWithProviders(); @@ -139,7 +137,7 @@ describe("LoggingSettings", () => { await waitFor(() => { expect(mockDeleteField).toHaveBeenCalled(); - expect(mockMutateAsync).toHaveBeenCalledWith( + expect(mockMutate).toHaveBeenCalledWith( { store_prompts_in_spend_logs: false, }, @@ -150,11 +148,11 @@ describe("LoggingSettings", () => { it("should show success notification on successful submission", async () => { const user = userEvent.setup(); - mockDeleteField.mockResolvedValue({ message: "Field deleted successfully" }); - mockMutateAsync.mockImplementation(async (_params, options) => { - await Promise.resolve(); + mockDeleteField.mockImplementation((_params, options) => { + options?.onSettled?.(); + }); + mockMutate.mockImplementation((_params, options) => { options?.onSuccess?.(); - return { message: "Success" }; }); renderWithProviders(); @@ -167,30 +165,11 @@ describe("LoggingSettings", () => { }); }); - it("should show error notification when submission throws", async () => { - const user = userEvent.setup(); - const error = new Error("Network error"); - mockMutateAsync.mockRejectedValue(error); - mockParseErrorMessage.mockReturnValue("Network error"); - - renderWithProviders(); - - const saveButton = screen.getByRole("button", { name: "Save Settings" }); - await user.click(saveButton); - - await waitFor(() => { - expect(mockNotificationsManager.fromBackend).toHaveBeenCalledWith( - "Failed to save spend logs settings: Network error", - ); - }); - }); - - it("should show error notification via onError callback", async () => { + it("should show a single error notification via onError callback", async () => { const user = userEvent.setup(); const error = new Error("Backend error"); - mockMutateAsync.mockImplementation((_params, options) => { + mockMutate.mockImplementation((_params, options) => { options?.onError?.(error); - return Promise.reject(error); }); mockParseErrorMessage.mockReturnValue("Backend error"); @@ -204,11 +183,12 @@ describe("LoggingSettings", () => { "Failed to save spend logs settings: Backend error", ); }); + expect(mockNotificationsManager.fromBackend).toHaveBeenCalledTimes(1); }); it("should show loading state on save button when update pending", () => { mockUseStoreRequestInSpendLogs.mockReturnValue({ - mutateAsync: mockMutateAsync, + mutate: mockMutate, isPending: true, } as any); @@ -221,7 +201,7 @@ describe("LoggingSettings", () => { it("should show loading state on save button when delete pending", () => { mockUseDeleteProxyConfigField.mockReturnValue({ - mutateAsync: mockDeleteField, + mutate: mockDeleteField, isPending: true, } as any); @@ -297,11 +277,12 @@ describe("LoggingSettings", () => { it("should continue with update even if deleteField fails", async () => { const user = userEvent.setup(); const deleteError = new Error("Field does not exist"); - mockDeleteField.mockRejectedValue(deleteError); - mockMutateAsync.mockImplementation(async (_params, options) => { - await Promise.resolve(); + mockDeleteField.mockImplementation((_params, options) => { + options?.onError?.(deleteError); + options?.onSettled?.(); + }); + mockMutate.mockImplementation((_params, options) => { options?.onSuccess?.(); - return { message: "Success" }; }); renderWithProviders(); @@ -311,7 +292,7 @@ describe("LoggingSettings", () => { await waitFor(() => { expect(mockDeleteField).toHaveBeenCalled(); - expect(mockMutateAsync).toHaveBeenCalledWith( + expect(mockMutate).toHaveBeenCalledWith( { store_prompts_in_spend_logs: false, }, @@ -323,11 +304,11 @@ describe("LoggingSettings", () => { it("should submit with only store prompts enabled when retention is empty", async () => { const user = userEvent.setup(); - mockDeleteField.mockResolvedValue({ message: "Field deleted successfully" }); - mockMutateAsync.mockImplementation(async (_params, options) => { - await Promise.resolve(); + mockDeleteField.mockImplementation((_params, options) => { + options?.onSettled?.(); + }); + mockMutate.mockImplementation((_params, options) => { options?.onSuccess?.(); - return { message: "Success" }; }); renderWithProviders(); @@ -340,7 +321,7 @@ describe("LoggingSettings", () => { await waitFor(() => { expect(mockDeleteField).toHaveBeenCalled(); - expect(mockMutateAsync).toHaveBeenCalledWith( + expect(mockMutate).toHaveBeenCalledWith( { store_prompts_in_spend_logs: true, }, diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/LoggingSettings/LoggingSettings.tsx b/ui/litellm-dashboard/src/components/Settings/AdminSettings/LoggingSettings/LoggingSettings.tsx index c3aaebd3bf..456240ec5d 100644 --- a/ui/litellm-dashboard/src/components/Settings/AdminSettings/LoggingSettings/LoggingSettings.tsx +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/LoggingSettings/LoggingSettings.tsx @@ -18,8 +18,8 @@ import React, { useMemo } from "react"; const LoggingSettings: React.FC = () => { const [form] = Form.useForm(); - const { mutateAsync, isPending } = useStoreRequestInSpendLogs(); - const { mutateAsync: deleteField, isPending: isDeletingField } = useDeleteProxyConfigField(); + const { mutate, isPending } = useStoreRequestInSpendLogs(); + const { mutate: deleteField, isPending: isDeletingField } = useDeleteProxyConfigField(); const { data: proxyConfigData, isLoading: isLoadingConfig } = useProxyConfig(ConfigType.GENERAL_SETTINGS); const storePromptsValue = Form.useWatch("store_prompts_in_spend_logs", form); @@ -42,44 +42,39 @@ const LoggingSettings: React.FC = () => { }; }, [proxyConfigData]); - const handleFormSubmit = async (formValues: StoreRequestInSpendLogsParams) => { - try { - const retentionPeriodValue = formValues.maximum_spend_logs_retention_period; - const shouldDeleteRetentionPeriod = - !retentionPeriodValue || - (typeof retentionPeriodValue === "string" && retentionPeriodValue.trim() === ""); + const handleFormSubmit = (formValues: StoreRequestInSpendLogsParams) => { + const retentionPeriodValue = formValues.maximum_spend_logs_retention_period; + const hasRetentionPeriod = + typeof retentionPeriodValue === "string" && retentionPeriodValue.trim() !== ""; - if (shouldDeleteRetentionPeriod) { - try { - await deleteField({ - config_type: ConfigType.GENERAL_SETTINGS, - field_name: GeneralSettingsFieldName.MAXIMUM_SPEND_LOGS_RETENTION_PERIOD, - }); - } catch (deleteError) { - console.warn("Failed to delete retention period field (may not exist):", deleteError); - } - } + const updateParams: StoreRequestInSpendLogsParams = { + store_prompts_in_spend_logs: formValues.store_prompts_in_spend_logs, + ...(hasRetentionPeriod && { maximum_spend_logs_retention_period: retentionPeriodValue }), + }; - const updateParams: StoreRequestInSpendLogsParams = { - store_prompts_in_spend_logs: formValues.store_prompts_in_spend_logs, - ...(retentionPeriodValue && - typeof retentionPeriodValue === "string" && - retentionPeriodValue.trim() !== "" && { - maximum_spend_logs_retention_period: retentionPeriodValue, - }), - }; - - await mutateAsync(updateParams, { - onSuccess: () => { - NotificationsManager.success("Spend logs settings updated successfully"); - }, - onError: (error) => { - NotificationsManager.fromBackend("Failed to save spend logs settings: " + parseErrorMessage(error)); - }, + const submitUpdate = () => + mutate(updateParams, { + onSuccess: () => NotificationsManager.success("Spend logs settings updated successfully"), + onError: (error) => + NotificationsManager.fromBackend("Failed to save spend logs settings: " + parseErrorMessage(error)), }); - } catch (error) { - NotificationsManager.fromBackend("Failed to save spend logs settings: " + parseErrorMessage(error)); + + if (hasRetentionPeriod) { + submitUpdate(); + return; } + + deleteField( + { + config_type: ConfigType.GENERAL_SETTINGS, + field_name: GeneralSettingsFieldName.MAXIMUM_SPEND_LOGS_RETENTION_PERIOD, + }, + { + onError: (deleteError) => + console.warn("Failed to delete retention period field (may not exist):", deleteError), + onSettled: submitUpdate, + }, + ); }; return (