Skip to content

refactor: internal users confirm settings modal #24306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ryan-crabbe wants to merge 6 commits into from
Closed

refactor: internal users confirm settings modal #24306

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c63ad09
feat(ui): add confirmation modal for destructive default user setting…
ryan-crabbe Mar 21, 2026
af75094
refactor(ui): type DefaultUserSettings values instead of using any
ryan-crabbe Mar 21, 2026
e34b7ec
fix(ui): use InputNumber for number-typed fields in default user sett…
ryan-crabbe Mar 21, 2026
30181aa
fix(ui): remove warning alert from confirmation modal
ryan-crabbe Mar 21, 2026
94c2270
fix(ui): keep confirmation modal open on API failure, restore respons…
ryan-crabbe Mar 21, 2026
bcfa665
fix(ui): stale closure in executeSave, type mismatch, modal dismiss d…
ryan-crabbe Mar 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
303 changes: 297 additions & 6 deletions ui/litellm-dashboard/src/components/DefaultUserSettings.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { act, fireEvent, render, screen, waitFor } from "@testing-library/react";
import { beforeEach, describe, expect, it, vi } from "vitest";
import DefaultUserSettings from "./DefaultUserSettings";
import DefaultUserSettings, { computeSettingsDiff } from "./DefaultUserSettings";
import * as networking from "./networking";

vi.mock("./networking", () => ({
Expand All @@ -20,6 +20,10 @@ vi.mock("./common_components/budget_duration_dropdown", () => ({
getBudgetDurationLabel: (value: string) => value,
}));

vi.mock("@/utils/dataUtils", () => ({
formatNumberWithCommas: (n: number) => String(n),
}));

vi.mock("./key_team_helpers/fetch_available_models_team_key", () => ({
getModelDisplayName: (model: string) => model,
}));
Expand Down Expand Up @@ -115,13 +119,11 @@ describe("DefaultUserSettings", () => {
expect(screen.queryByText("Edit Settings")).not.toBeInTheDocument();
});

it("should save settings when save button is clicked", async () => {
it("should save settings directly when no destructive changes", async () => {
// No changes at all → direct save (no modal)
mockGetInternalUserSettings.mockResolvedValue(mockSettings);
mockUpdateInternalUserSettings.mockResolvedValue({
settings: {
...mockSettings.values,
max_budget: 2000,
},
settings: mockSettings.values,
});

render(<DefaultUserSettings {...defaultProps} />);
Expand All @@ -148,6 +150,295 @@ describe("DefaultUserSettings", () => {
expect(mockUpdateInternalUserSettings).toHaveBeenCalled();
});

Comment on lines 120 to 152

@greptile-apps greptile-apps Bot Mar 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Test coverage weakened — API response reflection no longer verified

The original test verified that a successful save causes the new API-returned value to be reflected in the component state by using a mock response with max_budget: 2000. The update removes this, replacing it with a response that echoes the unchanged values. As a result, there is no longer a test that confirms the component correctly applies the API response (e.g., setSettings({ ...settings, values: updatedSettings.settings })) after a direct (non-modal) save. A regression in that state-update path would go undetected.

Consider keeping at least one test where the mock API response differs from the initial state to verify the settings are updated in the UI after saving.

Rule Used: What: Flag any modifications to existing tests and... (source)

// No modal should appear
expect(screen.queryByText("Review Changes")).not.toBeInTheDocument();
expect(screen.getByText("Edit Settings")).toBeInTheDocument();
});

it("should show confirmation modal when a team is removed", async () => {
const settingsWithTeams = {
...mockSettings,
values: {
...mockSettings.values,
teams: [
{ team_id: "team-alpha", max_budget_in_team: 50, user_role: "user" },
{ team_id: "team-beta", max_budget_in_team: 25, user_role: "admin" },
],
},
};
mockGetInternalUserSettings.mockResolvedValue(settingsWithTeams);

render(<DefaultUserSettings {...defaultProps} />);

await waitFor(() => {
expect(screen.getByText("Edit Settings")).toBeInTheDocument();
});

// Enter edit mode
act(() => {
fireEvent.click(screen.getByText("Edit Settings"));
});

// Remove the first team
const removeButtons = screen.getAllByText("Remove");
act(() => {
fireEvent.click(removeButtons[0]);
});

// Click Save Changes
act(() => {
fireEvent.click(screen.getByText("Save Changes"));
});

// Modal should appear
await waitFor(() => {
expect(screen.getByText("Review Changes")).toBeInTheDocument();
});

// Should mention the removed team
expect(screen.getByText(/team-alpha/)).toBeInTheDocument();

// Save should NOT have been called yet
expect(mockUpdateInternalUserSettings).not.toHaveBeenCalled();
});

it("should save after confirming in the modal", async () => {
const settingsWithTeams = {
...mockSettings,
values: {
...mockSettings.values,
teams: [
{ team_id: "team-alpha", max_budget_in_team: 50, user_role: "user" },
{ team_id: "team-beta", max_budget_in_team: 25, user_role: "admin" },
],
},
};
mockGetInternalUserSettings.mockResolvedValue(settingsWithTeams);
mockUpdateInternalUserSettings.mockResolvedValue({
settings: {
...settingsWithTeams.values,
teams: [{ team_id: "team-beta", max_budget_in_team: 25, user_role: "admin" }],
},
});

render(<DefaultUserSettings {...defaultProps} />);

await waitFor(() => {
expect(screen.getByText("Edit Settings")).toBeInTheDocument();
});

// Enter edit mode, remove first team, click Save
act(() => {
fireEvent.click(screen.getByText("Edit Settings"));
});
const removeButtons = screen.getAllByText("Remove");
act(() => {
fireEvent.click(removeButtons[0]);
});
act(() => {
fireEvent.click(screen.getByText("Save Changes"));
});

// Wait for modal
await waitFor(() => {
expect(screen.getByText("Review Changes")).toBeInTheDocument();
});

// Click Confirm
await act(async () => {
fireEvent.click(screen.getByText("Confirm Changes"));
});

// Save should have been called
await waitFor(() => {
expect(mockUpdateInternalUserSettings).toHaveBeenCalledTimes(1);
});

// The saved teams should not include team-alpha
const sentValues = mockUpdateInternalUserSettings.mock.calls[0][1];
const sentTeamIds = sentValues.teams.map((t: any) => t.team_id);
expect(sentTeamIds).not.toContain("team-alpha");
expect(sentTeamIds).toContain("team-beta");
});

it("should NOT save when modal is cancelled", async () => {
const settingsWithTeams = {
...mockSettings,
values: {
...mockSettings.values,
teams: [
{ team_id: "team-alpha", max_budget_in_team: 50, user_role: "user" },
],
},
};
mockGetInternalUserSettings.mockResolvedValue(settingsWithTeams);

render(<DefaultUserSettings {...defaultProps} />);

await waitFor(() => {
expect(screen.getByText("Edit Settings")).toBeInTheDocument();
});

// Enter edit mode, remove team, click Save
act(() => {
fireEvent.click(screen.getByText("Edit Settings"));
});
const removeButtons = screen.getAllByText("Remove");
act(() => {
fireEvent.click(removeButtons[0]);
});
act(() => {
fireEvent.click(screen.getByText("Save Changes"));
});

// Wait for modal
await waitFor(() => {
expect(screen.getByText("Review Changes")).toBeInTheDocument();
});

// Click Cancel inside the modal (there are two Cancel buttons: one in the
// edit header and one in the modal footer). The modal's Cancel is the last
// one rendered.
const cancelButtons = screen.getAllByText("Cancel");
await act(async () => {
fireEvent.click(cancelButtons[cancelButtons.length - 1]);
});
Comment on lines +302 to +305

@greptile-apps greptile-apps Bot Mar 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Fragile cancel button selector relying on DOM order

The test retrieves all buttons labelled "Cancel" and picks the last one, relying on rendering order to distinguish between the edit-header Cancel and the modal footer Cancel:

const cancelButtons = screen.getAllByText("Cancel");
await act(async () => {
  fireEvent.click(cancelButtons[cancelButtons.length - 1]);
});

If the component or modal ever renders an additional "Cancel" element (e.g., a tooltip, a third panel, or the order changes with Ant Design updates), this selector will silently click the wrong button and the test will give false confidence. Consider adding a data-testid to the modal footer Cancel button — or at minimum using a within(screen.getByRole("dialog")) scope:

const dialog = screen.getByRole("dialog");
const modalCancelButton = within(dialog).getByText("Cancel");
await act(async () => {
  fireEvent.click(modalCancelButton);
});


// Save should NOT have been called
expect(mockUpdateInternalUserSettings).not.toHaveBeenCalled();

// Should still be in edit mode (Save Changes button visible)
expect(screen.getByText("Save Changes")).toBeInTheDocument();
});
});

// ---------------------------------------------------------------------------
// Tests: computeSettingsDiff (pure function)
// ---------------------------------------------------------------------------

describe("computeSettingsDiff", () => {
it("returns no changes when values are identical", () => {
const values = { max_budget: 100, models: ["gpt-4"], teams: [] };
const { changes, hasDestructiveChanges } = computeSettingsDiff(values, { ...values });
expect(changes).toHaveLength(0);
expect(hasDestructiveChanges).toBe(false);
});

it("detects team removal as destructive", () => {
const original = {
teams: [
{ team_id: "team-alpha", user_role: "user" },
{ team_id: "team-beta", user_role: "admin" },
],
};
const edited = {
teams: [{ team_id: "team-alpha", user_role: "user" }],
};

const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited);
expect(hasDestructiveChanges).toBe(true);
expect(changes).toEqual(
expect.arrayContaining([
expect.objectContaining({
type: "removed",
details: expect.stringContaining("team-beta"),
}),
]),
);
});

it("detects team addition as non-destructive", () => {
const original = { teams: [] };
const edited = { teams: [{ team_id: "new-team", user_role: "user" }] };

const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited);
expect(hasDestructiveChanges).toBe(false);
expect(changes).toEqual(
expect.arrayContaining([expect.objectContaining({ type: "added" })]),
);
});

it("detects team budget change as destructive", () => {
const original = { teams: [{ team_id: "t1", max_budget_in_team: 100, user_role: "user" }] };
const edited = { teams: [{ team_id: "t1", max_budget_in_team: 50, user_role: "user" }] };

const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited);
expect(hasDestructiveChanges).toBe(true);
expect(changes[0].type).toBe("changed");
expect(changes[0].details).toContain("$100");
expect(changes[0].details).toContain("$50");
});

it("detects model removal as destructive", () => {
const original = { models: ["gpt-4", "gpt-3.5-turbo"] };
const edited = { models: ["gpt-4"] };

const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited);
expect(hasDestructiveChanges).toBe(true);
expect(changes).toEqual(
expect.arrayContaining([
expect.objectContaining({ type: "removed", details: expect.stringContaining("gpt-3.5-turbo") }),
]),
);
});

it("detects model addition as non-destructive", () => {
const original = { models: ["gpt-4"] };
const edited = { models: ["gpt-4", "claude-3"] };

const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited);
expect(hasDestructiveChanges).toBe(false);
expect(changes).toEqual(
expect.arrayContaining([expect.objectContaining({ type: "added" })]),
);
});

it("detects scalar field cleared as destructive", () => {
const original = { max_budget: 100 };
const edited = { max_budget: null };

const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited);
expect(hasDestructiveChanges).toBe(true);
expect(changes[0].type).toBe("removed");
expect(changes[0].details).toContain("Cleared");
});

it("detects scalar field set as non-destructive", () => {
const original = { max_budget: null };
const edited = { max_budget: 200 };

const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited);
expect(hasDestructiveChanges).toBe(false);
expect(changes[0].type).toBe("added");
});

it("detects scalar value change as destructive", () => {
const original = { user_role: "internal_user" };
const edited = { user_role: "internal_user_view_only" };

const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited);
expect(hasDestructiveChanges).toBe(true);
expect(changes[0].type).toBe("changed");
});

it("handles string team_ids in original", () => {
const original = { teams: ["team-alpha", "team-beta"] };
const edited = { teams: [{ team_id: "team-alpha", user_role: "user" }] };

const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited);
expect(hasDestructiveChanges).toBe(true);
expect(changes).toEqual(
expect.arrayContaining([
expect.objectContaining({ type: "removed", details: expect.stringContaining("team-beta") }),
]),
);
});

it("treats null and undefined as equal for scalars", () => {
const original = { max_budget: null };
const edited = { max_budget: undefined };

const { changes } = computeSettingsDiff(original, edited);
expect(changes).toHaveLength(0);
});
});
Loading
Loading