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 { ThemeSelector } from "./ThemeSelector";
|
||||
import { CustomModelDropdown } from "./CustomModelDropdown";
|
||||
import { applyPresetToSelection, generatePresetId, validatePresetId } from "../utils/modelPresets";
|
||||
import { applyPresetToSelection, generateUniquePresetId } from "../utils/modelPresets";
|
||||
|
||||
/**
|
||||
* Settings sections configuration.
|
||||
@@ -98,7 +98,6 @@ export function SettingsModal({
|
||||
const [testNotificationLoading, setTestNotificationLoading] = useState(false);
|
||||
const [editingPresetId, setEditingPresetId] = useState<string | null>(null);
|
||||
const [presetDraft, setPresetDraft] = useState<ModelPreset | null>(null);
|
||||
const [presetIdTouched, setPresetIdTouched] = useState(false);
|
||||
|
||||
// Backup state
|
||||
const [backupInfo, setBackupInfo] = useState<BackupListResponse | null>(null);
|
||||
@@ -427,17 +426,20 @@ export function SettingsModal({
|
||||
const savePresetDraft = () => {
|
||||
if (!presetDraft) return;
|
||||
|
||||
const nextId = presetDraft.id.trim();
|
||||
const nextName = presetDraft.name.trim();
|
||||
if (!nextName || !nextId || !validatePresetId(nextId)) {
|
||||
addToast("Preset name is required and ID must be 1–32 letters, numbers, hyphens, or underscores", "error");
|
||||
if (!nextName) {
|
||||
addToast("Preset name is required", "error");
|
||||
return;
|
||||
}
|
||||
|
||||
const presets = form.modelPresets || [];
|
||||
if (presets.some((preset) => preset.id === nextId && preset.id !== editingPresetId)) {
|
||||
addToast("Preset ID must be unique", "error");
|
||||
return;
|
||||
|
||||
// For new presets, generate unique ID from name; for edits, keep existing ID
|
||||
let nextId: string;
|
||||
if (editingPresetId) {
|
||||
nextId = editingPresetId;
|
||||
} else {
|
||||
nextId = generateUniquePresetId(nextName, presets);
|
||||
}
|
||||
|
||||
const normalizedDraft: ModelPreset = {
|
||||
@@ -459,7 +461,6 @@ export function SettingsModal({
|
||||
|
||||
setEditingPresetId(null);
|
||||
setPresetDraft(null);
|
||||
setPresetIdTouched(false);
|
||||
};
|
||||
|
||||
/** Render a scope indicator banner for the current section */
|
||||
@@ -785,7 +786,6 @@ export function SettingsModal({
|
||||
onClick={() => {
|
||||
setEditingPresetId(preset.id);
|
||||
setPresetDraft({ ...preset });
|
||||
setPresetIdTouched(true);
|
||||
}}
|
||||
>
|
||||
Edit
|
||||
@@ -807,7 +807,6 @@ export function SettingsModal({
|
||||
if (editingPresetId === preset.id) {
|
||||
setEditingPresetId(null);
|
||||
setPresetDraft(null);
|
||||
setPresetIdTouched(false);
|
||||
}
|
||||
}}
|
||||
>
|
||||
@@ -826,7 +825,6 @@ export function SettingsModal({
|
||||
onClick={() => {
|
||||
setEditingPresetId(null);
|
||||
setPresetDraft({ id: "", name: "", executorProvider: undefined, executorModelId: undefined, validatorProvider: undefined, validatorModelId: undefined });
|
||||
setPresetIdTouched(false);
|
||||
}}
|
||||
>
|
||||
Add Preset
|
||||
@@ -845,31 +843,10 @@ export function SettingsModal({
|
||||
value={presetDraft.name}
|
||||
onChange={(e) => {
|
||||
const name = e.target.value;
|
||||
setPresetDraft((current) => current ? {
|
||||
...current,
|
||||
name,
|
||||
id: presetIdTouched ? current.id : generatePresetId(name),
|
||||
} : current);
|
||||
setPresetDraft((current) => current ? { ...current, name } : current);
|
||||
}}
|
||||
/>
|
||||
</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 ? (
|
||||
<small>No models available. Configure authentication first.</small>
|
||||
) : (
|
||||
@@ -930,7 +907,7 @@ export function SettingsModal({
|
||||
)}
|
||||
<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-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>
|
||||
) : null}
|
||||
|
||||
@@ -523,7 +523,6 @@ describe("SettingsModal", () => {
|
||||
await user.click(screen.getByText("Add Preset"));
|
||||
|
||||
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"));
|
||||
|
||||
@@ -3,6 +3,7 @@ import type { ModelPreset } from "@fusion/core";
|
||||
import {
|
||||
applyPresetToSelection,
|
||||
generatePresetId,
|
||||
generateUniquePresetId,
|
||||
getPresetByName,
|
||||
getRecommendedPresetForSize,
|
||||
validatePresetId,
|
||||
@@ -70,4 +71,50 @@ describe("modelPresets utils", () => {
|
||||
expect(generatePresetId("!!!")).toBe("preset");
|
||||
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";
|
||||
}
|
||||
|
||||
/**
|
||||
* 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(
|
||||
buildApp(),
|
||||
"PUT",
|
||||
@@ -5459,8 +5459,56 @@ describe("PUT /settings", () => {
|
||||
{ "Content-Type": "application/json" },
|
||||
);
|
||||
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body.error).toContain("duplicate id");
|
||||
expect(res.status).toBe(200);
|
||||
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 () => {
|
||||
|
||||
@@ -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 {
|
||||
if (value === undefined) return undefined;
|
||||
if (!Array.isArray(value)) {
|
||||
@@ -111,17 +122,31 @@ function validateModelPresets(value: unknown): ModelPreset[] | undefined {
|
||||
}
|
||||
|
||||
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`);
|
||||
|
||||
if (!id) {
|
||||
throw new Error(`modelPresets[${index}].id is required`);
|
||||
}
|
||||
if (!name) {
|
||||
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)) {
|
||||
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);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user