refactor: internal users confirm settings modal#24306
refactor: internal users confirm settings modal#24306ryan-crabbe wants to merge 6 commits intolitellm_ryan_march_20from
Conversation
…s changes When removing teams, clearing budgets, or changing roles in Default User Settings, a "Review Changes" modal now appears summarizing what will change before persisting. Non-destructive changes (additions only) save directly.
Replace any-typed settings state with DefaultUserSettingsValues and SettingsResponse interfaces matching the backend's Python types. Simplifies normalizeTeams and computeSettingsDiff significantly.
…ings Adds a type === "number" case in renderEditableField so max_budget renders as an InputNumber instead of a free-text TextInput. Prevents string input and fixes null displaying as "null" in the field.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR adds a "Review Changes" confirmation modal to the Default User Settings page that intercepts destructive edits (team removals, budget clears, role changes) before they are persisted, while non-destructive changes (additions) continue to save directly. The refactor also tightens the component's TypeScript types, introduces a pure Key changes:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/components/DefaultUserSettings.tsx | Core refactor introducing the confirmation flow. The executeSave / handleConfirmSave / handleSaveSettings split is well-structured and the stale-closure fix (setSettings((prev) => ...)) is correct. Two concerns: the asymmetric team_id guard between old/new team loops in computeSettingsDiff can produce spurious "removed" entries triggering unwanted modal appearances; and the new module-level normalizeTeams dropped the invalid-object fallback from the original component-scoped version. |
| ui/litellm-dashboard/src/components/DefaultUserSettings.test.tsx | Test coverage is significantly expanded and correctly addresses prior gaps: modal flow (show, confirm, cancel), API failure keeping modal open, and API-response reflection. The computeSettingsDiff unit tests are well-written as pure-function tests. Minor fragility in the cancel-button selector (uses array position instead of scoped query). |
| ui/litellm-dashboard/src/components/common_components/ConfirmSettingsChangeModal.tsx | New modal component with color-coded change entries. Correctly blocks all dismissal paths during save via keyboard and maskClosable props. Clean and self-contained. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User clicks Save Changes] --> B[processedValues = editedValues with empty→null]
B --> C[computeSettingsDiff original vs processed]
C --> D{hasDestructiveChanges?}
D -- No --> E[executeSave directly]
D -- Yes --> F[setPendingChanges / setPendingProcessedValues\nsetShowConfirmModal = true]
F --> G[ConfirmSettingsChangeModal shown\nwith color-coded change list]
G --> H{User action}
H -- Cancel --> I[setShowConfirmModal=false\nclear pending state\nremain in edit mode]
H -- Confirm --> J[handleConfirmSave → executeSave]
E --> K{API call}
J --> K
K -- Success → true --> L[setSettings functional update\nsetIsEditing=false\nif modal: clear pending state]
K -- Failure → false --> M[NotificationManager error toast\nmodal stays open / edit mode preserved]
Last reviewed commit: "fix(ui): stale closu..."
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const handleConfirmSave = async () => { | ||
| if (!pendingProcessedValues) return; | ||
| await executeSave(pendingProcessedValues); | ||
| setShowConfirmModal(false); | ||
| setPendingChanges([]); | ||
| setPendingProcessedValues(null); | ||
| }; |
There was a problem hiding this comment.
executeSave catches errors internally and never re-throws them. This means the await executeSave(...) call in handleConfirmSave always resolves without throwing, so the modal is unconditionally closed (lines 216–218) even when the API call failed. The user is left in editing mode with their pending changes gone, forcing them to re-edit from scratch.
const handleConfirmSave = async () => {
if (!pendingProcessedValues) return;
await executeSave(pendingProcessedValues);
// ↑ executeSave swallows errors — this always runs even on failure
setShowConfirmModal(false); // modal closes on error
setPendingChanges([]); // pending changes lost on error
setPendingProcessedValues(null);
};A simple fix is to have executeSave return a success boolean (or re-throw), and only clear modal state on success:
// In executeSave, return true/false based on success
const executeSave = async (processedValues: DefaultUserSettingsValues): Promise<boolean> => {
if (!accessToken) return false;
setSaving(true);
try {
const updatedSettings = await updateInternalUserSettings(accessToken, processedValues);
setSettings({ ...settings, values: updatedSettings.settings });
setIsEditing(false);
return true;
} catch (error) {
console.error("Error updating SSO settings:", error);
NotificationManager.fromBackend("Failed to update settings: " + error);
return false;
} finally {
setSaving(false);
}
};
const handleConfirmSave = async () => {
if (!pendingProcessedValues) return;
const success = await executeSave(pendingProcessedValues);
if (success) {
setShowConfirmModal(false);
setPendingChanges([]);
setPendingProcessedValues(null);
}
};| /** | ||
| * Compare original settings values against edited values and return a list of | ||
| * human-readable changes. A change is "destructive" if something was removed, | ||
| * cleared, or reduced. | ||
| */ | ||
| export function computeSettingsDiff( | ||
| original: DefaultUserSettingsValues, | ||
| edited: DefaultUserSettingsValues, | ||
| ): { changes: SettingsChange[]; hasDestructiveChanges: boolean } { | ||
| const changes: SettingsChange[] = []; | ||
|
|
||
| // --- Teams --- | ||
| const oldTeams = normalizeTeams(original.teams || []); | ||
| const newTeams = normalizeTeams(edited.teams || []); | ||
| const oldTeamMap = new Map(oldTeams.map((t) => [t.team_id, t])); | ||
| const newTeamIds = new Set(newTeams.map((t) => t.team_id)); | ||
|
|
||
| for (const t of oldTeams) { | ||
| if (!newTeamIds.has(t.team_id)) { | ||
| changes.push({ field: "Teams", type: "removed", details: `Team "${t.team_id}" removed` }); | ||
| } | ||
| } | ||
| for (const t of newTeams) { | ||
| if (!t.team_id) continue; | ||
| const old = oldTeamMap.get(t.team_id); | ||
| if (!old) { | ||
| changes.push({ field: "Teams", type: "added", details: `Team "${t.team_id}" added` }); | ||
| continue; | ||
| } | ||
| if (old.max_budget_in_team !== t.max_budget_in_team) { | ||
| const fmt = (v?: number) => (v !== undefined ? `$${v}` : "No limit"); | ||
| changes.push({ field: "Teams", type: "changed", details: `Team "${t.team_id}" budget: ${fmt(old.max_budget_in_team)} → ${fmt(t.max_budget_in_team)}` }); | ||
| } | ||
| if (old.user_role !== t.user_role) { | ||
| changes.push({ field: "Teams", type: "changed", details: `Team "${t.team_id}" role: ${old.user_role} → ${t.user_role}` }); | ||
| } | ||
| } | ||
|
|
||
| // --- Models --- | ||
| const oldModels = new Set(original.models || []); | ||
| const newModels = new Set(edited.models || []); | ||
| const removedModels = [...oldModels].filter((m) => !newModels.has(m)); | ||
| const addedModels = [...newModels].filter((m) => !oldModels.has(m)); | ||
| if (removedModels.length) changes.push({ field: "Models", type: "removed", details: `${removedModels.join(", ")} removed` }); | ||
| if (addedModels.length) changes.push({ field: "Models", type: "added", details: `${addedModels.join(", ")} added` }); | ||
|
|
||
| // --- Scalars --- | ||
| const scalarKeys = ["user_role", "max_budget", "budget_duration"] as const; | ||
| for (const key of scalarKeys) { | ||
| const oldVal = original[key] ?? null; | ||
| const newVal = edited[key] ?? null; | ||
| if (oldVal === newVal) continue; | ||
|
|
||
| const displayName = key.replace(/_/g, " ").replace(/\b\w/g, (l) => l.toUpperCase()); | ||
| if (oldVal != null && newVal == null) { | ||
| changes.push({ field: displayName, type: "removed", details: `Cleared (was "${oldVal}")` }); | ||
| } else if (oldVal == null && newVal != null) { | ||
| changes.push({ field: displayName, type: "added", details: `Set to "${newVal}"` }); | ||
| } else { | ||
| changes.push({ field: displayName, type: "changed", details: `"${oldVal}" → "${newVal}"` }); | ||
| } | ||
| } | ||
|
|
||
| return { changes, hasDestructiveChanges: changes.some((c) => c.type === "removed" || c.type === "changed") }; |
There was a problem hiding this comment.
The computeSettingsDiff doc comment says: "A change is 'destructive' if something was removed, cleared, or reduced." However, the implementation classifies all scalar changes — including budget increases — as "changed", which is then treated as destructive (line 114). For example, changing max_budget from 100 to 200 triggers the confirmation modal, even though it is a non-harmful increase.
The same applies to team budget increases via old.max_budget_in_team !== t.max_budget_in_team (line 80), which fires "changed" regardless of direction.
Consider differentiating between increases (non-destructive) and decreases (destructive) for numeric fields, or update the doc comment to reflect the conservative intent so the discrepancy is explicit.
| @@ -148,6 +150,295 @@ describe("DefaultUserSettings", () => { | |||
| expect(mockUpdateInternalUserSettings).toHaveBeenCalled(); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
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)
…e test executeSave now returns a boolean; handleConfirmSave only clears modal state on success. Adds tests for the error path and for verifying the UI reflects API response values after a successful save.
| ); | ||
|
|
||
| const updatedSettings = await updateInternalUserSettings(accessToken, processedValues); | ||
| setSettings({ ...settings, values: updatedSettings.settings }); |
There was a problem hiding this comment.
settings after async save
executeSave captures settings from its closure at the time the function is defined. After await updateInternalUserSettings(...) resolves, the captured settings may be stale if any concurrent React state update modified it during the network round-trip (e.g., the confirmation modal being opened a second time or a parallel re-fetch). Using the stale snapshot when spreading means field_schema and other keys could silently revert to old values.
Use the functional form of setSettings to guarantee the latest state is used:
| setSettings({ ...settings, values: updatedSettings.settings }); | |
| setSettings((prev) => ({ ...prev, values: updatedSettings.settings })); |
| } else if (type === "number") { | ||
| return ( | ||
| <InputNumber | ||
| style={{ width: "100%" }} | ||
| value={editedValues[key] as number | undefined} | ||
| onChange={(value) => handleTextInputChange(key, value)} | ||
| placeholder={property.description || ""} | ||
| className="mt-2" | ||
| min={0} | ||
| step={0.01} | ||
| precision={2} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
handleTextInputChange signature
This PR changed handleTextInputChange from (key: string, value: any) to (key: keyof DefaultUserSettingsValues, value: ...). However, renderEditableField still receives key: string (from Object.entries(field_schema.properties) on line 546), so every handleTextInputChange(key, value) call inside it is now a type error — including this new number branch and all the pre-existing branches (lines ~327, ~345, ~352, ~362, ~376, ~409, ~425).
The cleanest fix is to cast key at the call-site using a type assertion inside renderEditableField:
const renderEditableField = (key: string, property: any, value: any) => {
const typedKey = key as keyof DefaultUserSettingsValues;
// … then use typedKey everywhere instead of key when calling handleTextInputChange
Alternatively, change renderEditableField's signature to (key: keyof DefaultUserSettingsValues, ...) and update the call-site in renderSettings accordingly.
| <Modal | ||
| title="Review Changes" | ||
| open={isOpen} | ||
| onOk={onConfirm} | ||
| onCancel={onCancel} | ||
| confirmLoading={confirmLoading} | ||
| okText={confirmLoading ? "Saving..." : "Confirm Changes"} | ||
| cancelText="Cancel" | ||
| okButtonProps={{ disabled: confirmLoading }} | ||
| cancelButtonProps={{ disabled: confirmLoading }} |
There was a problem hiding this comment.
cancelButtonProps={{ disabled: confirmLoading }} only disables the footer cancel button. Ant Design's Modal still allows dismissal via the Esc key and backdrop click by default — both keyboard and maskClosable default to true.
If the user dismisses this way while confirmLoading is true:
onCancelfires, clearingpendingProcessedValuesandpendingChanges.- The in-flight
executeSavecontinues, and on success callssetSettings(...)plussetIsEditing(false). - The user, believing they cancelled, is silently taken out of edit mode when the save completes, with no visible feedback.
To block all dismissal paths while saving is in progress, set maskClosable and keyboard on the Modal to false when confirmLoading is true.
…uring save - Use functional state updater in executeSave to avoid stale settings closure - Widen handleTextInputChange key param to string (schema keys are dynamic) - Block Esc/backdrop dismiss on confirmation modal during confirmLoading
| for (const t of oldTeams) { | ||
| if (!newTeamIds.has(t.team_id)) { | ||
| changes.push({ field: "Teams", type: "removed", details: `Team "${t.team_id}" removed` }); | ||
| } | ||
| } |
There was a problem hiding this comment.
team_id guard between old/new teams loops
The newTeams loop on line 74 guards against empty team_id entries with if (!t.team_id) continue, but the oldTeams loop (lines 68–71) has no equivalent guard:
for (const t of oldTeams) {
if (!newTeamIds.has(t.team_id)) { // no guard for empty t.team_id
changes.push({ field: "Teams", type: "removed", details: `Team "${t.team_id}" removed` });
}
}If the API ever returns a team object with an empty/missing team_id (or if normalizeTeams produces one for a malformed entry — the new module-level normalizeTeams dropped the old typeof team === "object" && team.team_id guard), oldTeams would contain a { team_id: undefined } entry. Since newTeamIds wouldn't have undefined, this would push a spurious "Team "undefined" removed" entry into changes, set hasDestructiveChanges: true, and trigger the confirmation modal even when no real team was removed.
A simple fix mirrors the pattern already used for newTeams:
| for (const t of oldTeams) { | |
| if (!newTeamIds.has(t.team_id)) { | |
| changes.push({ field: "Teams", type: "removed", details: `Team "${t.team_id}" removed` }); | |
| } | |
| } | |
| for (const t of oldTeams) { | |
| if (!t.team_id) continue; | |
| if (!newTeamIds.has(t.team_id)) { | |
| changes.push({ field: "Teams", type: "removed", details: `Team "${t.team_id}" removed` }); | |
| } | |
| } |
| const cancelButtons = screen.getAllByText("Cancel"); | ||
| await act(async () => { | ||
| fireEvent.click(cancelButtons[cancelButtons.length - 1]); | ||
| }); |
There was a problem hiding this comment.
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);
});| function normalizeTeams(teams: (string | TeamEntry)[]): TeamEntry[] { | ||
| return teams.map((team) => | ||
| typeof team === "string" | ||
| ? { team_id: team, user_role: "user" as const } | ||
| : { team_id: team.team_id, max_budget_in_team: team.max_budget_in_team, user_role: team.user_role || "user" }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
normalizeTeams drops the invalid-object fallback
The old component-scoped normalizeTeams had an explicit guard and fallback for malformed team objects:
// OLD
} else if (typeof team === "object" && team.team_id) {
return { team_id: team.team_id, ... };
}
return { team_id: "", user_role: "user" as const }; // fallback for objects without team_idThe new module-level version unconditionally spreads the object:
// NEW
: { team_id: team.team_id, max_budget_in_team: team.max_budget_in_team, user_role: team.user_role || "user" }If the API returns a team object where team_id is absent or null (malformed data), team.team_id resolves to undefined. This flows into both oldTeams (where the missing !t.team_id guard described above is the direct consequence) and editedValues, potentially producing { team_id: undefined } entries that render as "Team ID: " in the editor. Consider restoring the guard:
| function normalizeTeams(teams: (string | TeamEntry)[]): TeamEntry[] { | |
| return teams.map((team) => | |
| typeof team === "string" | |
| ? { team_id: team, user_role: "user" as const } | |
| : { team_id: team.team_id, max_budget_in_team: team.max_budget_in_team, user_role: team.user_role || "user" }, | |
| ); | |
| } | |
| function normalizeTeams(teams: (string | TeamEntry)[]): TeamEntry[] { | |
| return teams | |
| .map((team) => | |
| typeof team === "string" | |
| ? { team_id: team, user_role: "user" as const } | |
| : { team_id: team.team_id, max_budget_in_team: team.max_budget_in_team, user_role: team.user_role || "user" }, | |
| ) | |
| .filter((t) => !!t.team_id); | |
| } |
2 participants
Type
🧹 Refactoring
Changes
When removing teams, clearing budgets, or changing roles in Default User Settings, a "Review Changes" modal now appears summarizing what will change before persisting. Non-destructive changes (additions only) save directly.