Fix KVEditor duplicate key rename validation
Prevent users from renaming a KVEditor entry to an existing key and show inline validation errors.
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
import { Button, Input, tokens } from "@fluentui/react-components";
|
||||
import { Button, Input, MessageBar, MessageBarBody, tokens } from "@fluentui/react-components";
|
||||
import { Add24Regular, Delete24Regular } from "@fluentui/react-icons";
|
||||
import { useEffect, useMemo, useState } from "react";
|
||||
import { useConfigStyles } from "./configStyles";
|
||||
|
||||
interface KVEditorProps {
|
||||
@@ -9,14 +10,63 @@ interface KVEditorProps {
|
||||
|
||||
export function KVEditor({ entries, onChange }: KVEditorProps): React.JSX.Element {
|
||||
const styles = useConfigStyles();
|
||||
const rows = Object.entries(entries);
|
||||
const rows = useMemo(() => Object.entries(entries), [entries]);
|
||||
const entryKeys = useMemo(() => Object.keys(entries), [entries]);
|
||||
const entryKeyList = entryKeys.join(",");
|
||||
const [editedKeys, setEditedKeys] = useState<Record<string, string>>(
|
||||
Object.fromEntries(rows.map(([key]) => [key, key])),
|
||||
);
|
||||
const [errors, setErrors] = useState<Record<string, string>>({});
|
||||
|
||||
const handleKeyChange = (oldKey: string, newKey: string): void => {
|
||||
const next: Record<string, string> = {};
|
||||
for (const [k, v] of Object.entries(entries)) {
|
||||
next[k === oldKey ? newKey : k] = v;
|
||||
useEffect(() => {
|
||||
setEditedKeys(Object.fromEntries(rows.map(([key]) => [key, key])));
|
||||
setErrors((previousErrors) =>
|
||||
Object.fromEntries(
|
||||
entryKeys
|
||||
.filter((key) => previousErrors[key])
|
||||
.map((key) => [key, previousErrors[key] ?? ""]),
|
||||
),
|
||||
);
|
||||
}, [entryKeyList, rows, entryKeys]);
|
||||
|
||||
const validateKey = (oldKey: string, newKey: string): string | null => {
|
||||
const trimmedKey = newKey.trim();
|
||||
if (trimmedKey.length === 0) {
|
||||
return "Key may not be blank.";
|
||||
}
|
||||
|
||||
const conflict = Object.entries(editedKeys).some(
|
||||
([otherKey, otherValue]) => otherKey !== oldKey && otherValue === trimmedKey,
|
||||
);
|
||||
if (conflict) {
|
||||
return "A setting with this key already exists.";
|
||||
}
|
||||
|
||||
return null;
|
||||
};
|
||||
|
||||
const updateKeyValue = (oldKey: string, newKey: string): void => {
|
||||
setEditedKeys((previous) => ({ ...previous, [oldKey]: newKey }));
|
||||
setErrors((previous) =>
|
||||
Object.fromEntries(Object.entries(previous).filter(([key]) => key !== oldKey)),
|
||||
);
|
||||
};
|
||||
|
||||
const commitKeyChange = (oldKey: string): void => {
|
||||
const newKey = editedKeys[oldKey] ?? oldKey;
|
||||
const error = validateKey(oldKey, newKey);
|
||||
if (error) {
|
||||
setErrors((previous) => ({ ...previous, [oldKey]: error }));
|
||||
return;
|
||||
}
|
||||
|
||||
if (newKey !== oldKey) {
|
||||
const next: Record<string, string> = {};
|
||||
for (const [key, value] of Object.entries(entries)) {
|
||||
next[key === oldKey ? newKey : key] = value;
|
||||
}
|
||||
onChange(next);
|
||||
}
|
||||
onChange(next);
|
||||
};
|
||||
|
||||
const handleValueChange = (key: string, value: string): void => {
|
||||
@@ -43,11 +93,13 @@ export function KVEditor({ entries, onChange }: KVEditorProps): React.JSX.Elemen
|
||||
{rows.map(([key, value]) => (
|
||||
<div key={key} className={styles.fieldRow}>
|
||||
<Input
|
||||
value={key}
|
||||
value={editedKeys[key] ?? key}
|
||||
size="small"
|
||||
style={{ width: 160, fontFamily: tokens.fontFamilyMonospace }}
|
||||
aria-label={`Setting name: ${key}`}
|
||||
onChange={(_e, d) => { handleKeyChange(key, d.value); }}
|
||||
aria-invalid={Boolean(errors[key])}
|
||||
onBlur={() => { commitKeyChange(key); }}
|
||||
onChange={(_e, d) => { updateKeyValue(key, d.value); }}
|
||||
/>
|
||||
<Input
|
||||
value={value}
|
||||
@@ -63,6 +115,11 @@ export function KVEditor({ entries, onChange }: KVEditorProps): React.JSX.Elemen
|
||||
onClick={() => { handleDelete(key); }}
|
||||
aria-label={`Delete key ${key}`}
|
||||
/>
|
||||
{errors[key] ? (
|
||||
<MessageBar intent="error" className={styles.fieldError}>
|
||||
<MessageBarBody>{errors[key]}</MessageBarBody>
|
||||
</MessageBar>
|
||||
) : null}
|
||||
</div>
|
||||
))}
|
||||
<Button
|
||||
|
||||
37
frontend/src/components/config/__tests__/KVEditor.test.tsx
Normal file
37
frontend/src/components/config/__tests__/KVEditor.test.tsx
Normal file
@@ -0,0 +1,37 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { fireEvent, render, screen } from "@testing-library/react";
|
||||
import { FluentProvider, webLightTheme } from "@fluentui/react-components";
|
||||
import { KVEditor } from "../KVEditor";
|
||||
|
||||
describe("KVEditor", () => {
|
||||
it("prevents renaming a key to a duplicate and shows an inline error", () => {
|
||||
const handleChange = vi.fn();
|
||||
render(
|
||||
<FluentProvider theme={webLightTheme}>
|
||||
<KVEditor entries={{ first: "1", second: "2" }} onChange={handleChange} />
|
||||
</FluentProvider>,
|
||||
);
|
||||
|
||||
const firstKeyInput = screen.getByLabelText(/Setting name: first/i);
|
||||
fireEvent.change(firstKeyInput, { target: { value: "second" } });
|
||||
fireEvent.blur(firstKeyInput);
|
||||
|
||||
expect(handleChange).not.toHaveBeenCalled();
|
||||
expect(screen.getByText(/already exists/i)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("commits a valid key rename on blur", () => {
|
||||
const handleChange = vi.fn();
|
||||
render(
|
||||
<FluentProvider theme={webLightTheme}>
|
||||
<KVEditor entries={{ first: "1", second: "2" }} onChange={handleChange} />
|
||||
</FluentProvider>,
|
||||
);
|
||||
|
||||
const firstKeyInput = screen.getByLabelText(/Setting name: first/i);
|
||||
fireEvent.change(firstKeyInput, { target: { value: "primary" } });
|
||||
fireEvent.blur(firstKeyInput);
|
||||
|
||||
expect(handleChange).toHaveBeenCalledWith({ primary: "1", second: "2" });
|
||||
});
|
||||
});
|
||||
@@ -167,6 +167,11 @@ export const useConfigStyles = makeStyles({
|
||||
fontFamily: "monospace",
|
||||
borderLeft: `3px solid ${tokens.colorBrandStroke1}`,
|
||||
},
|
||||
fieldError: {
|
||||
gridColumn: "1 / -1",
|
||||
marginTop: tokens.spacingVerticalXXS,
|
||||
color: tokens.colorPaletteRedForeground1,
|
||||
},
|
||||
logLine: {
|
||||
padding: `${tokens.spacingVerticalXS} ${tokens.spacingHorizontalS}`,
|
||||
borderRadius: tokens.borderRadiusSmall,
|
||||
|
||||
Reference in New Issue
Block a user