feat(FN-800): auto-generate model preset IDs and remove manual ID editing
- Add slugify and generatePresetId utilities with collision handling for unique preset IDs - Remove manual ID input field from preset editor UI in SettingsModal - Auto-generate preset IDs server-side from name on creation - Simplify Header tests by removing ID-related test cases - Deduplicate tablet header controls tests by importing from shared Header tests - Remove unused CSS rules for removed ID input field - Update reviewer to pass through preset ID generation logic
This commit is contained in:
@@ -6,7 +6,7 @@ import type { AuthProvider, ModelInfo, BackupListResponse, SettingsExportData }
|
|||||||
import type { ToastType } from "../hooks/useToast";
|
import type { ToastType } from "../hooks/useToast";
|
||||||
import { ThemeSelector } from "./ThemeSelector";
|
import { ThemeSelector } from "./ThemeSelector";
|
||||||
import { CustomModelDropdown } from "./CustomModelDropdown";
|
import { CustomModelDropdown } from "./CustomModelDropdown";
|
||||||
import { applyPresetToSelection, generatePresetId, validatePresetId } from "../utils/modelPresets";
|
import { applyPresetToSelection, generateUniquePresetId } from "../utils/modelPresets";
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Settings sections configuration.
|
* Settings sections configuration.
|
||||||
@@ -98,7 +98,6 @@ export function SettingsModal({
|
|||||||
const [testNotificationLoading, setTestNotificationLoading] = useState(false);
|
const [testNotificationLoading, setTestNotificationLoading] = useState(false);
|
||||||
const [editingPresetId, setEditingPresetId] = useState<string | null>(null);
|
const [editingPresetId, setEditingPresetId] = useState<string | null>(null);
|
||||||
const [presetDraft, setPresetDraft] = useState<ModelPreset | null>(null);
|
const [presetDraft, setPresetDraft] = useState<ModelPreset | null>(null);
|
||||||
const [presetIdTouched, setPresetIdTouched] = useState(false);
|
|
||||||
|
|
||||||
// Backup state
|
// Backup state
|
||||||
const [backupInfo, setBackupInfo] = useState<BackupListResponse | null>(null);
|
const [backupInfo, setBackupInfo] = useState<BackupListResponse | null>(null);
|
||||||
@@ -427,17 +426,20 @@ export function SettingsModal({
|
|||||||
const savePresetDraft = () => {
|
const savePresetDraft = () => {
|
||||||
if (!presetDraft) return;
|
if (!presetDraft) return;
|
||||||
|
|
||||||
const nextId = presetDraft.id.trim();
|
|
||||||
const nextName = presetDraft.name.trim();
|
const nextName = presetDraft.name.trim();
|
||||||
if (!nextName || !nextId || !validatePresetId(nextId)) {
|
if (!nextName) {
|
||||||
addToast("Preset name is required and ID must be 1–32 letters, numbers, hyphens, or underscores", "error");
|
addToast("Preset name is required", "error");
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
const presets = form.modelPresets || [];
|
const presets = form.modelPresets || [];
|
||||||
if (presets.some((preset) => preset.id === nextId && preset.id !== editingPresetId)) {
|
|
||||||
addToast("Preset ID must be unique", "error");
|
// For new presets, generate unique ID from name; for edits, keep existing ID
|
||||||
return;
|
let nextId: string;
|
||||||
|
if (editingPresetId) {
|
||||||
|
nextId = editingPresetId;
|
||||||
|
} else {
|
||||||
|
nextId = generateUniquePresetId(nextName, presets);
|
||||||
}
|
}
|
||||||
|
|
||||||
const normalizedDraft: ModelPreset = {
|
const normalizedDraft: ModelPreset = {
|
||||||
@@ -459,7 +461,6 @@ export function SettingsModal({
|
|||||||
|
|
||||||
setEditingPresetId(null);
|
setEditingPresetId(null);
|
||||||
setPresetDraft(null);
|
setPresetDraft(null);
|
||||||
setPresetIdTouched(false);
|
|
||||||
};
|
};
|
||||||
|
|
||||||
/** Render a scope indicator banner for the current section */
|
/** Render a scope indicator banner for the current section */
|
||||||
@@ -785,7 +786,6 @@ export function SettingsModal({
|
|||||||
onClick={() => {
|
onClick={() => {
|
||||||
setEditingPresetId(preset.id);
|
setEditingPresetId(preset.id);
|
||||||
setPresetDraft({ ...preset });
|
setPresetDraft({ ...preset });
|
||||||
setPresetIdTouched(true);
|
|
||||||
}}
|
}}
|
||||||
>
|
>
|
||||||
Edit
|
Edit
|
||||||
@@ -807,7 +807,6 @@ export function SettingsModal({
|
|||||||
if (editingPresetId === preset.id) {
|
if (editingPresetId === preset.id) {
|
||||||
setEditingPresetId(null);
|
setEditingPresetId(null);
|
||||||
setPresetDraft(null);
|
setPresetDraft(null);
|
||||||
setPresetIdTouched(false);
|
|
||||||
}
|
}
|
||||||
}}
|
}}
|
||||||
>
|
>
|
||||||
@@ -826,7 +825,6 @@ export function SettingsModal({
|
|||||||
onClick={() => {
|
onClick={() => {
|
||||||
setEditingPresetId(null);
|
setEditingPresetId(null);
|
||||||
setPresetDraft({ id: "", name: "", executorProvider: undefined, executorModelId: undefined, validatorProvider: undefined, validatorModelId: undefined });
|
setPresetDraft({ id: "", name: "", executorProvider: undefined, executorModelId: undefined, validatorProvider: undefined, validatorModelId: undefined });
|
||||||
setPresetIdTouched(false);
|
|
||||||
}}
|
}}
|
||||||
>
|
>
|
||||||
Add Preset
|
Add Preset
|
||||||
@@ -845,31 +843,10 @@ export function SettingsModal({
|
|||||||
value={presetDraft.name}
|
value={presetDraft.name}
|
||||||
onChange={(e) => {
|
onChange={(e) => {
|
||||||
const name = e.target.value;
|
const name = e.target.value;
|
||||||
setPresetDraft((current) => current ? {
|
setPresetDraft((current) => current ? { ...current, name } : current);
|
||||||
...current,
|
|
||||||
name,
|
|
||||||
id: presetIdTouched ? current.id : generatePresetId(name),
|
|
||||||
} : current);
|
|
||||||
}}
|
}}
|
||||||
/>
|
/>
|
||||||
</div>
|
</div>
|
||||||
<div className="form-group">
|
|
||||||
<label htmlFor="preset-id">ID</label>
|
|
||||||
<input
|
|
||||||
id="preset-id"
|
|
||||||
type="text"
|
|
||||||
value={presetDraft.id}
|
|
||||||
onChange={(e) => {
|
|
||||||
setPresetIdTouched(true);
|
|
||||||
setPresetDraft((current) => current ? { ...current, id: e.target.value } : current);
|
|
||||||
}}
|
|
||||||
/>
|
|
||||||
{presetDraft.id && !validatePresetId(presetDraft.id) ? (
|
|
||||||
<small className="field-error">ID must be 1–32 letters, numbers, hyphens, or underscores</small>
|
|
||||||
) : (
|
|
||||||
<small>Slug-friendly unique identifier used for preset mappings.</small>
|
|
||||||
)}
|
|
||||||
</div>
|
|
||||||
{availableModels.length === 0 ? (
|
{availableModels.length === 0 ? (
|
||||||
<small>No models available. Configure authentication first.</small>
|
<small>No models available. Configure authentication first.</small>
|
||||||
) : (
|
) : (
|
||||||
@@ -930,7 +907,7 @@ export function SettingsModal({
|
|||||||
)}
|
)}
|
||||||
<div className="modal-actions" style={{ justifyContent: "flex-start" }}>
|
<div className="modal-actions" style={{ justifyContent: "flex-start" }}>
|
||||||
<button type="button" className="btn btn-primary btn-sm" onClick={savePresetDraft}>Save preset</button>
|
<button type="button" className="btn btn-primary btn-sm" onClick={savePresetDraft}>Save preset</button>
|
||||||
<button type="button" className="btn btn-sm" onClick={() => { setEditingPresetId(null); setPresetDraft(null); setPresetIdTouched(false); }}>Cancel</button>
|
<button type="button" className="btn btn-sm" onClick={() => { setEditingPresetId(null); setPresetDraft(null); }}>Cancel</button>
|
||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
) : null}
|
) : null}
|
||||||
|
|||||||
@@ -523,7 +523,6 @@ describe("SettingsModal", () => {
|
|||||||
await user.click(screen.getByText("Add Preset"));
|
await user.click(screen.getByText("Add Preset"));
|
||||||
|
|
||||||
await user.type(screen.getByLabelText("Name"), "Budget");
|
await user.type(screen.getByLabelText("Name"), "Budget");
|
||||||
expect((screen.getByLabelText("ID") as HTMLInputElement).value).toBe("budget");
|
|
||||||
|
|
||||||
await user.click(screen.getByText("Save preset"));
|
await user.click(screen.getByText("Save preset"));
|
||||||
await user.click(screen.getByText("Save"));
|
await user.click(screen.getByText("Save"));
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ import type { ModelPreset } from "@fusion/core";
|
|||||||
import {
|
import {
|
||||||
applyPresetToSelection,
|
applyPresetToSelection,
|
||||||
generatePresetId,
|
generatePresetId,
|
||||||
|
generateUniquePresetId,
|
||||||
getPresetByName,
|
getPresetByName,
|
||||||
getRecommendedPresetForSize,
|
getRecommendedPresetForSize,
|
||||||
validatePresetId,
|
validatePresetId,
|
||||||
@@ -70,4 +71,50 @@ describe("modelPresets utils", () => {
|
|||||||
expect(generatePresetId("!!!")).toBe("preset");
|
expect(generatePresetId("!!!")).toBe("preset");
|
||||||
expect(generatePresetId("a".repeat(40))).toBe("a".repeat(32));
|
expect(generatePresetId("a".repeat(40))).toBe("a".repeat(32));
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("generateUniquePresetId", () => {
|
||||||
|
it("returns the base slug when no collision", () => {
|
||||||
|
// "standard" is not in the presets fixture
|
||||||
|
expect(generateUniquePresetId("Standard", presets)).toBe("standard");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns base slug when existing list is empty", () => {
|
||||||
|
expect(generateUniquePresetId("Budget", [])).toBe("budget");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("appends suffix when base slug is already taken", () => {
|
||||||
|
// "budget" is already used in presets, so should get "budget-1"
|
||||||
|
expect(generateUniquePresetId("Budget", presets)).toBe("budget-1");
|
||||||
|
// "complex" is also taken, so should get "complex-1"
|
||||||
|
expect(generateUniquePresetId("Complex", presets)).toBe("complex-1");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("increments suffix until finding a free id", () => {
|
||||||
|
const crowded: ModelPreset[] = [
|
||||||
|
{ id: "budget", name: "Budget" },
|
||||||
|
{ id: "budget-1", name: "Budget Copy" },
|
||||||
|
{ id: "budget-2", name: "Budget Copy 2" },
|
||||||
|
];
|
||||||
|
expect(generateUniquePresetId("Budget", crowded)).toBe("budget-3");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("truncates base slug to leave room for suffix", () => {
|
||||||
|
const longName = "a".repeat(40);
|
||||||
|
const existing: ModelPreset[] = [
|
||||||
|
{ id: generatePresetId(longName), name: longName },
|
||||||
|
];
|
||||||
|
const result = generateUniquePresetId(longName, existing);
|
||||||
|
// baseId is 32 a's, collision → truncate to 28 a's + "-1" = 30 chars
|
||||||
|
expect(result).toBe(`${"a".repeat(28)}-1`);
|
||||||
|
expect(result.length).toBeLessThanOrEqual(32);
|
||||||
|
expect(validatePresetId(result)).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("handles fallback 'preset' slug collisions", () => {
|
||||||
|
const existing: ModelPreset[] = [
|
||||||
|
{ id: "preset", name: "!!!" },
|
||||||
|
];
|
||||||
|
expect(generateUniquePresetId("!!!", existing)).toBe("preset-1");
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -45,3 +45,26 @@ export function generatePresetId(name: string): string {
|
|||||||
|
|
||||||
return slug || "preset";
|
return slug || "preset";
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Generate a unique preset ID derived from the preset name, avoiding collisions
|
||||||
|
* with existing preset IDs. If the base slug is already taken, appends `-1`,
|
||||||
|
* `-2`, etc. until a unique ID is found.
|
||||||
|
*/
|
||||||
|
export function generateUniquePresetId(name: string, existingPresets: ModelPreset[]): string {
|
||||||
|
const baseId = generatePresetId(name);
|
||||||
|
const takenIds = new Set(existingPresets.map((p) => p.id));
|
||||||
|
|
||||||
|
if (!takenIds.has(baseId)) return baseId;
|
||||||
|
|
||||||
|
// Leave room for the numeric suffix (-N)
|
||||||
|
const maxBase = 30;
|
||||||
|
let candidate = baseId;
|
||||||
|
let idx = 1;
|
||||||
|
while (takenIds.has(candidate) && idx < 100) {
|
||||||
|
const suffix = `-${idx}`;
|
||||||
|
candidate = `${baseId.slice(0, maxBase - suffix.length)}${suffix}`;
|
||||||
|
idx++;
|
||||||
|
}
|
||||||
|
return candidate;
|
||||||
|
}
|
||||||
|
|||||||
@@ -5450,7 +5450,7 @@ describe("PUT /settings", () => {
|
|||||||
}));
|
}));
|
||||||
});
|
});
|
||||||
|
|
||||||
it("rejects duplicate preset ids", async () => {
|
it("resolves duplicate preset ids by auto-generating unique ids", async () => {
|
||||||
const res = await REQUEST(
|
const res = await REQUEST(
|
||||||
buildApp(),
|
buildApp(),
|
||||||
"PUT",
|
"PUT",
|
||||||
@@ -5459,8 +5459,56 @@ describe("PUT /settings", () => {
|
|||||||
{ "Content-Type": "application/json" },
|
{ "Content-Type": "application/json" },
|
||||||
);
|
);
|
||||||
|
|
||||||
expect(res.status).toBe(400);
|
expect(res.status).toBe(200);
|
||||||
expect(res.body.error).toContain("duplicate id");
|
expect(store.updateSettings).toHaveBeenCalledWith(expect.objectContaining({
|
||||||
|
modelPresets: [
|
||||||
|
expect.objectContaining({ id: "budget", name: "Budget" }),
|
||||||
|
// "budget" collides; falls back to slug of name "Budget 2" → "budget-2"
|
||||||
|
expect.objectContaining({ id: "budget-2", name: "Budget 2" }),
|
||||||
|
],
|
||||||
|
}));
|
||||||
|
});
|
||||||
|
|
||||||
|
it("auto-generates preset id from name when id is omitted", async () => {
|
||||||
|
const updatedSettings = {
|
||||||
|
...DEFAULT_SETTINGS,
|
||||||
|
modelPresets: [{ id: "my-custom-preset", name: "My Custom Preset" }],
|
||||||
|
};
|
||||||
|
(store.updateSettings as ReturnType<typeof vi.fn>).mockResolvedValue(updatedSettings);
|
||||||
|
|
||||||
|
const res = await REQUEST(
|
||||||
|
buildApp(),
|
||||||
|
"PUT",
|
||||||
|
"/api/settings",
|
||||||
|
JSON.stringify({ modelPresets: [{ name: "My Custom Preset" }] }),
|
||||||
|
{ "Content-Type": "application/json" },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(store.updateSettings).toHaveBeenCalledWith(expect.objectContaining({
|
||||||
|
modelPresets: [expect.objectContaining({ id: "my-custom-preset", name: "My Custom Preset" })],
|
||||||
|
}));
|
||||||
|
});
|
||||||
|
|
||||||
|
it("preserves explicit preset id when provided", async () => {
|
||||||
|
const updatedSettings = {
|
||||||
|
...DEFAULT_SETTINGS,
|
||||||
|
modelPresets: [{ id: "custom-id", name: "My Preset" }],
|
||||||
|
};
|
||||||
|
(store.updateSettings as ReturnType<typeof vi.fn>).mockResolvedValue(updatedSettings);
|
||||||
|
|
||||||
|
const res = await REQUEST(
|
||||||
|
buildApp(),
|
||||||
|
"PUT",
|
||||||
|
"/api/settings",
|
||||||
|
JSON.stringify({ modelPresets: [{ id: "custom-id", name: "My Preset" }] }),
|
||||||
|
{ "Content-Type": "application/json" },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(store.updateSettings).toHaveBeenCalledWith(expect.objectContaining({
|
||||||
|
modelPresets: [expect.objectContaining({ id: "custom-id", name: "My Preset" })],
|
||||||
|
}));
|
||||||
});
|
});
|
||||||
|
|
||||||
it("rejects incomplete model provider/modelId pairs", async () => {
|
it("rejects incomplete model provider/modelId pairs", async () => {
|
||||||
|
|||||||
@@ -97,6 +97,17 @@ function assertConsistentOptionalPair(
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function slugifyPresetName(name: string): string {
|
||||||
|
const slug = name
|
||||||
|
.trim()
|
||||||
|
.toLowerCase()
|
||||||
|
.replace(/[^a-z0-9_-]+/g, "-")
|
||||||
|
.replace(/-+/g, "-")
|
||||||
|
.replace(/^[-_]+|[-_]+$/g, "")
|
||||||
|
.slice(0, 32);
|
||||||
|
return slug || "preset";
|
||||||
|
}
|
||||||
|
|
||||||
function validateModelPresets(value: unknown): ModelPreset[] | undefined {
|
function validateModelPresets(value: unknown): ModelPreset[] | undefined {
|
||||||
if (value === undefined) return undefined;
|
if (value === undefined) return undefined;
|
||||||
if (!Array.isArray(value)) {
|
if (!Array.isArray(value)) {
|
||||||
@@ -111,17 +122,31 @@ function validateModelPresets(value: unknown): ModelPreset[] | undefined {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const candidate = preset as Record<string, unknown>;
|
const candidate = preset as Record<string, unknown>;
|
||||||
const id = validateOptionalModelField(candidate.id, `modelPresets[${index}].id`);
|
const rawId = validateOptionalModelField(candidate.id, `modelPresets[${index}].id`);
|
||||||
const name = validateOptionalModelField(candidate.name, `modelPresets[${index}].name`);
|
const name = validateOptionalModelField(candidate.name, `modelPresets[${index}].name`);
|
||||||
|
|
||||||
if (!id) {
|
|
||||||
throw new Error(`modelPresets[${index}].id is required`);
|
|
||||||
}
|
|
||||||
if (!name) {
|
if (!name) {
|
||||||
throw new Error(`modelPresets[${index}].name is required`);
|
throw new Error(`modelPresets[${index}].name is required`);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Auto-generate ID from name when not provided
|
||||||
|
let id = rawId || slugifyPresetName(name);
|
||||||
|
|
||||||
|
// If the explicit ID collides, fall back to the slugified name
|
||||||
if (seenIds.has(id)) {
|
if (seenIds.has(id)) {
|
||||||
throw new Error(`modelPresets contains duplicate id: ${id}`);
|
const slugId = slugifyPresetName(name);
|
||||||
|
if (!seenIds.has(slugId)) {
|
||||||
|
id = slugId;
|
||||||
|
} else {
|
||||||
|
// Both explicit ID and slug collide — append -1, -2, etc.
|
||||||
|
const maxBase = 30;
|
||||||
|
let idx = 1;
|
||||||
|
while (seenIds.has(id) && idx < 100) {
|
||||||
|
const suffix = `-${idx}`;
|
||||||
|
id = `${slugId.slice(0, maxBase - suffix.length)}${suffix}`;
|
||||||
|
idx++;
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
seenIds.add(id);
|
seenIds.add(id);
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user