Harden quick-add against review findings

Independent review of the quick-add range surfaced real holes:

- The emptyLabel guard suppressed creation for any name that happened
  to be a substring of the label — typing "Rent" in the parent picker
  (a substring of "No parent (root)") silently offered nothing. The
  guard is gone; the exact-match rule already suppresses creates when
  the full label is typed.
- Async creates resolved against click-time snapshots, so a checkbox
  toggled or chip removed during the server round trip was silently
  reverted. Consumers now apply functional updates or a latest-value
  ref.
- Created names are capped at 200 characters, matching the registry
  forms; over-long text fails inline instead of minting a permanent
  multi-kilobyte name.
- A create failing after the user blurred mid-flight reopens the list
  so the error is never invisible, and option rows are locked while a
  create is in flight so a race cannot override an explicit pick.
This commit is contained in:
Lars Nolden
2026-09-14 12:20:33 +02:00
parent c569ae7dbf
commit 676065292e
3 changed files with 38 additions and 16 deletions
+14 -8
View File
@@ -695,6 +695,11 @@ function CorrectionEditor({
mutate: Mutate; mutate: Mutate;
onChange: (value: CorrectionValue) => void; onChange: (value: CorrectionValue) => void;
}) { }) {
// Async creates resolve against the freshest correction, not the snapshot
// captured when the create row was clicked: a chip removed during the
// server round trip must survive the create landing.
const latest = useRef(value);
latest.current = value;
const addable = data.tags const addable = data.tags
.filter((t) => !value.tag_ids.includes(t.id)) .filter((t) => !value.tag_ids.includes(t.id))
.map((t) => ({ value: t.id, label: t.name })); .map((t) => ({ value: t.id, label: t.name }));
@@ -739,7 +744,9 @@ function CorrectionEditor({
mutate={mutate} mutate={mutate}
value={value.category_id} value={value.category_id}
disabled={disabled} disabled={disabled}
onChange={(category_id) => onChange({ ...value, category_id })} onChange={(category_id) =>
onChange({ ...latest.current, category_id })
}
/> />
</dd> </dd>
</div> </div>
@@ -783,14 +790,13 @@ function CorrectionEditor({
{ {
key: "tag", key: "tag",
label: `Create tag "${text}"`, label: `Create tag "${text}"`,
run: async () => run: async () => {
const id = await createTag(mutate, data, text);
onChange({ onChange({
...value, ...latest.current,
tag_ids: [ tag_ids: [...latest.current.tag_ids, id],
...value.tag_ids, });
await createTag(mutate, data, text), },
],
}),
}, },
] ]
} }
+4 -2
View File
@@ -460,7 +460,9 @@ function TransactionEditor({
required required
mutate={mutate} mutate={mutate}
value={value.category_id || ""} value={value.category_id || ""}
onChange={(category_id) => setValue({ ...value, category_id })} onChange={(category_id) =>
setValue((v) => ({ ...v, category_id }))
}
/> />
</Field> </Field>
)} )}
@@ -473,7 +475,7 @@ function TransactionEditor({
<TagPicker <TagPicker
data={data} data={data}
value={value.tag_ids} value={value.tag_ids}
onChange={(tag_ids) => setValue({ ...value, tag_ids })} onChange={(tag_ids) => setValue((v) => ({ ...v, tag_ids }))}
mutate={mutate} mutate={mutate}
/> />
<details open> <details open>
+20 -6
View File
@@ -179,6 +179,9 @@ export function Combobox({
setOpen(false); setOpen(false);
} catch (err) { } catch (err) {
setCreateError(err instanceof Error ? err.message : String(err)); setCreateError(err instanceof Error ? err.message : String(err));
// A blur may have closed the list mid-flight; a failure must never
// land invisibly.
setOpen(true);
} finally { } finally {
setCreating(false); setCreating(false);
} }
@@ -208,7 +211,11 @@ export function Combobox({
setActive(-1); setActive(-1);
setOpen(true); setOpen(true);
}} }}
onBlur={() => setOpen(false)} onBlur={() => {
// A blur during an in-flight create keeps the list mounted so the
// outcome (or the error row) stays visible.
if (!creating) setOpen(false);
}}
onKeyDown={(e) => { onKeyDown={(e) => {
if (e.key === "Escape") setOpen(false); if (e.key === "Escape") setOpen(false);
if ((e.key === "ArrowDown" || e.key === "ArrowUp") && open && total) { if ((e.key === "ArrowDown" || e.key === "ArrowUp") && open && total) {
@@ -250,6 +257,7 @@ export function Combobox({
} }
role="option" role="option"
aria-selected={o.value === value} aria-selected={o.value === value}
disabled={creating}
onMouseDown={(e) => e.preventDefault()} onMouseDown={(e) => e.preventDefault()}
onClick={() => pick(o.value)} onClick={() => pick(o.value)}
> >
@@ -565,6 +573,8 @@ export async function createTag(
data: Dataset, data: Dataset,
name: string, name: string,
): Promise<string> { ): Promise<string> {
if (name.length > 200)
throw new Error("Tag names are limited to 200 characters.");
const next = await mutate( const next = await mutate(
"/api/tags", "/api/tags",
{ tag: { id: "", name, hint: "" } }, { tag: { id: "", name, hint: "" } },
@@ -582,6 +592,8 @@ export async function createCategory(
data: Dataset, data: Dataset,
category: { name: string; parent_id: string; kind: string }, category: { name: string; parent_id: string; kind: string },
): Promise<string> { ): Promise<string> {
if (category.name.length > 200)
throw new Error("Category names are limited to 200 characters.");
const next = await mutate( const next = await mutate(
"/api/categories", "/api/categories",
{ category: { id: "", hint: "", ...category } }, { category: { id: "", hint: "", ...category } },
@@ -610,6 +622,11 @@ export function TagPicker({
const [draft, setDraft] = useState(""); const [draft, setDraft] = useState("");
const [busy, setBusy] = useState(false); const [busy, setBusy] = useState(false);
const [error, setError] = useState(""); const [error, setError] = useState("");
// The async add resolves against the freshest selection, not the one
// captured at click time: a checkbox toggled during the server round trip
// must survive the create landing.
const latest = useRef(value);
latest.current = value;
const add = async () => { const add = async () => {
const name = draft.trim(); const name = draft.trim();
if (!name || busy || !mutate) return; if (!name || busy || !mutate) return;
@@ -626,7 +643,7 @@ export function TagPicker({
setError(""); setError("");
try { try {
const id = await createTag(mutate, data, name); const id = await createTag(mutate, data, name);
onChange([...value, id]); onChange([...latest.current, id]);
setDraft(""); setDraft("");
} catch (err) { } catch (err) {
setError(err instanceof Error ? err.message : String(err)); setError(err instanceof Error ? err.message : String(err));
@@ -756,10 +773,7 @@ export function CategoryCombobox({
return data.categories.some((c) => pathOf(c.id) === full); return data.categories.some((c) => pathOf(c.id) === full);
}; };
const create = (text: string): ComboCreate[] => { const create = (text: string): ComboCreate[] => {
// Text aiming at the empty option ("No parent…") is a selection, not a if (!mutate) return [];
// new category name.
if (!mutate || emptyLabel?.toLowerCase().includes(text.toLowerCase()))
return [];
const segments = text const segments = text
.split("/") .split("/")
.map((s) => s.trim()) .map((s) => s.trim())