Merge pull request 'fix(review): #88 thickness band derived; #90 MultiSelect names + commit' (#97) from work/ux-bugs-fixes into main

This commit was merged in pull request #97.
This commit is contained in:
2026-09-21 17:01:49 +00:00
6 changed files with 244 additions and 32 deletions
+4 -1
View File
@@ -121,7 +121,10 @@ hard stop. Switch to a supported browser.
3. **Wood categories.** Pick at least one from the synced list.
4. **Edge type, figure, grade.** Pick from the synced lists. One edge type, at least one figure,
exactly one grade.
5. **Thickness band.** Pick the Woo band that the measured thickness rounds up into.
5. **Thickness band.** Not a control — it is derived automatically from the measured
thickness (the thickness-in rounds up into the synced Woo band, `TV-9` / `QUALITY-95`).
It displays read-only on the Review screen; fix it upstream by correcting the thickness
on capture/measure if the band looks wrong.
6. **Moisture.** Keep the default (kiln-dried) or change it.
7. **fig-* tags.** Optional; pick any from the synced list.
8. **feat-* tags.** Zero or more. Only pick existing `feat-*` tags.
+5 -2
View File
@@ -55,8 +55,11 @@ Cites REVIEW-5, REVIEW-15, REVIEW-20, METADATA-45.
with an "AI" badge; below it the field starts empty. The badge is presentation only and clears
on edit.
- **Required set.** Exactly one species and at least one wood-category leaf; one Edge Type, at
least one Figure, exactly one Grade, one Thickness band, one Moisture; fig-* and feat-* tags
optional; price; title, short_description, and description.
least one Figure, exactly one Grade, one Moisture; fig-* and feat-* tags optional; price;
title, short_description, and description. The **Thickness band is not an owner control** — it
is derived deterministically from `thickness_in` (the value rounds up into the synced Woo band,
`TV-9` / `QUALITY-95`), displayed read-only, and satisfies the readiness requirement
(`REVIEW-5`) via that derived value.
- **Primary category.** The owner declares one primary wood category and any secondary ones; the
default is the first selected.
- **Inline nudges.** Every empty required field shows an inline nudge as the owner works; there is
+41 -4
View File
@@ -9,7 +9,7 @@
// (METADATA-10: "parents group only and are never assigned"); only the
// leaf rows are selectable. Filtering hides a parent group when none of
// its leaves match.
import { createEventDispatcher } from 'svelte';
import { createEventDispatcher, onDestroy } from 'svelte';
export interface Opt { id: number; name: string; ai: boolean }
export interface Group { parent: { id: number; name: string }; leaves: Opt[] }
@@ -23,16 +23,30 @@
export let placeholder = 'Choose…';
export let testid = '';
export let onToggle: (id: number) => void;
// #90: commit (persist + re-review) after a toggle settles. The parent wires this
// so multi-select changes flush to the slab immediately — no need to touch another
// field first. Debounced so a quick multi-tap doesn't fire a commit per click.
export let onCommit: (() => void) | undefined = undefined;
const dispatch = createEventDispatcher();
let open = false;
let filter = '';
let focused: number | null = null;
let commitTimer: ReturnType<typeof setTimeout> | undefined;
const grouped = groups.length > 0;
// Effective selectable set (leaves when grouped, options when flat).
$: allOpts = (grouped ? groups.flatMap((g) => g.leaves) : options) as Opt[];
// #90: the closed state must show WHAT is selected (names), not just a count.
// Truncate long lists so the button stays one line.
$: selectedNames = allOpts.filter((o) => selected.includes(o.id)).map((o) => o.name);
$: summary =
selectedNames.length === 0
? ''
: selectedNames.length <= 2
? selectedNames.join(', ')
: `${selectedNames.slice(0, 2).join(', ')} +${selectedNames.length - 2}`;
$: q = filter.trim().toLowerCase();
$: filteredFlat = q ? allOpts.filter((o) => o.name.toLowerCase().includes(q)) : allOpts;
// Grouped: keep a parent only if at least one of its leaves survives the filter.
@@ -44,24 +58,46 @@
$: count = selected.length;
$: allFilteredSelected = filteredFlat.length > 0 && filteredFlat.every((o) => selected.includes(o.id));
function scheduleCommit() {
if (typeof onCommit !== 'function') return;
if (commitTimer) clearTimeout(commitTimer);
commitTimer = setTimeout(() => { commitTimer = undefined; onCommit(); }, 250);
}
function toggle(id: number) {
onToggle(id);
focused = id;
scheduleCommit(); // #90: persist + re-review without waiting for another field
}
function selectAll() {
for (const o of filteredFlat) if (!selected.includes(o.id)) onToggle(o.id);
scheduleCommit();
}
function clearAll() {
for (const o of [...filteredFlat]) if (selected.includes(o.id)) onToggle(o.id);
scheduleCommit();
}
function close() {
open = false;
filter = '';
if (commitTimer) { clearTimeout(commitTimer); commitTimer = undefined; }
if (typeof onCommit === 'function') onCommit(); // flush immediately on Done
dispatch('commit');
}
function close() { open = false; filter = ''; dispatch('commit'); }
function onKey(e: KeyboardEvent) { if (e.key === 'Escape' && open) open = false; }
function destroy() { if (commitTimer) clearTimeout(commitTimer); }
onDestroy(destroy);
</script>
<div class="ms" class:open data-testid={testid} on:keydown={onKey}>
<button type="button" class="ms-btn" data-testid={testid ? `${testid}-btn` : 'ms-btn'} on:click={() => (open = !open)} aria-expanded={open}>
<span class="ms-label">{open ? 'Choose ' + label.toLowerCase() : placeholder}</span>
<span class="ms-c">{count === 1 ? '1 selected' : `${count} selected`}</span>
<span class="ms-label" data-testid={testid ? `${testid}-summary` : 'ms-summary'}>
{open
? 'Choose ' + label.toLowerCase()
: summary !== ''
? summary
: placeholder}
</span>
<span class="ms-c">{count === 0 ? '' : (count === 1 ? '1 selected' : `${count} selected`)}</span>
<span class="ms-arrow" aria-hidden="true">{open ? '▴' : '▾'}</span>
</button>
@@ -129,6 +165,7 @@
.ms { border: 1px solid #444; border-radius: 3px; background: #1a1a1a; overflow: hidden; }
.ms-btn { width: 100%; display: flex; justify-content: space-between; align-items: center; gap: 8px;
padding: 9px 10px; background: #1d1d1d; border: 0; color: #eee; font-size: .9rem; cursor: pointer; text-align: left; }
.ms-btn .ms-label { flex: 1; min-width: 0; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; color: #eee; }
.ms-btn .ms-c { color: #00e5ff; font-weight: 700; font-size: .8rem; white-space: nowrap; }
.ms-arrow { color: #777; }
.ms-panel { border-top: 1px solid #333; background: #161616; }
+79 -25
View File
@@ -1,5 +1,5 @@
<script lang="ts">
import { onMount, onDestroy } from 'svelte';
import { onMount, onDestroy, tick } from 'svelte';
import {
getSlab, updateSlab, reviewSlab, getTaxonomy, getSettings, inferTaxon, inferContent,
ApiError,
@@ -7,6 +7,7 @@
} from '../../api/slabs';
import type { CalibratedCapture } from '../flow';
import MultiSelect from './MultiSelect.svelte';
import { deriveThicknessTermId } from './thicknessBand';
// Props handed off by App (one-column flow, UX.md §1).
export let slabId: string;
@@ -21,7 +22,7 @@
let loading = true;
// Picker option types (id + name + whether the AI recommended it).
interface Opt { id: number; name: string; ai: boolean }
interface Opt { id: number; name: string; ai: boolean; lowIn?: number }
const byId = <T extends { id: number }>(arr: T[]) => {
const m = new Map<number, T>();
for (const x of arr) m.set(x.id, x);
@@ -54,7 +55,13 @@
const attr = tax!.attributes.find(
(a) => a.name.toLowerCase() === name.toLowerCase(),
);
return (attr?.terms ?? []).map((t) => ({ id: t.id, name: t.name, ai: false }));
return (attr?.terms ?? []).map((t) => {
// Carry the band's numeric lower bound for thickness (TV-9 round-up); other
// attributes ignore it. "2-4 in" → 2; a name with no leading number → undefined.
const m = t.name.match(/\d+(?:[./]\d+)?/);
const lowIn = m ? Number(m[0]) : undefined;
return { id: t.id, name: t.name, ai: false, lowIn: Number.isFinite(lowIn) ? lowIn : undefined };
});
};
speciesOpts = childrenOf(speciesGrouper).map((c) => ({ id: c.id, name: c.name, ai: false }));
// Wood categories: parent/child grouped (METADATA-10 — parents group only, never
@@ -82,12 +89,33 @@
}
// --- Editable model (mirrors the live slab) ---
let speciesId: number | '' = '';
// #90: single-selects bind a STRING var to the native <select>, because
// <option value={id}> coerces the option value to a string. Binding a
// `number | ''` var directly to those string values is the classic
// number/string mismatch that leaves the selected option blank after AI
// prefill or reload. `Number(...)` at commit normalizes back.
let speciesSel: string = '';
let edgeSel: string = '';
let gradeSel: string = '';
let woodCatIds: number[] = [];
let edgeTypeId: number | '' = '';
let figureIds: number[] = [];
let gradeId: number | '' = '';
let thicknessId: number | '' = '';
// #88: thickness band is NOT an owner control — it is derived deterministically
// from thickness_in (TV-9 / QUALITY-95) against the synced Woo thickness terms.
// The Review screen shows it read-only; the owner changes thickness upstream, and
// the band follows.
$: thicknessId = deriveThicknessTermId(
slab?.thickness_in ?? null,
thicknessOpts.map((o) => ({ id: o.id, name: o.name })),
);
$: thicknessName = thicknessOpts.find((o) => o.id === thicknessId)?.name ?? '';
// #88 display text: the derived band name, or an explicit "no band" note when a
// thickness is set but nothing in the synced set encloses it.
$: thicknessDisplay =
thicknessId !== ''
? thicknessName
: (slab?.thickness_in ?? 0) > 0
? `— no band for ${fmt1(slab!.thickness_in!)}″`
: '—';
let moistureId: number | '' = '';
let figTagIds: number[] = [];
let featTagIds: number[] = [];
@@ -129,12 +157,13 @@
const fmt2 = (v: number) => v.toFixed(2);
function hydrate(s: Slab, t: Taxonomy | null) {
speciesId = s.species_id ?? '';
speciesSel = s.species_id != null ? String(s.species_id) : '';
woodCatIds = [...(s.wood_category_ids ?? [])];
edgeTypeId = s.edge_type_term_id ?? '';
edgeSel = s.edge_type_term_id != null ? String(s.edge_type_term_id) : '';
figureIds = [...(s.figure_term_ids ?? [])];
gradeId = s.grade_term_id ?? '';
thicknessId = s.thickness_term_id ?? '';
gradeSel = s.grade_term_id != null ? String(s.grade_term_id) : '';
// thicknessId is derived reactively from slab.thickness_in (#88) — not hydrated
// from the stored term, so a stale stored band can't shadow the derived value.
moistureId = s.moisture_term_id ?? '';
figTagIds = [...(s.fig_tag_ids ?? [])];
featTagIds = [...(s.feat_tag_ids ?? [])];
@@ -165,11 +194,11 @@
reviewError = '';
try {
const patch = {
species_id: speciesId === '' ? null : Number(speciesId),
species_id: speciesSel === '' ? null : Number(speciesSel),
wood_category_ids: woodCatIds,
edge_type_term_id: edgeTypeId === '' ? null : Number(edgeTypeId),
edge_type_term_id: edgeSel === '' ? null : Number(edgeSel),
figure_term_ids: figureIds,
grade_term_id: gradeId === '' ? null : Number(gradeId),
grade_term_id: gradeSel === '' ? null : Number(gradeSel),
thickness_term_id: thicknessId === '' ? null : Number(thicknessId),
moisture_term_id: moistureId === '' ? null : Number(moistureId),
fig_tag_ids: figTagIds,
@@ -218,13 +247,13 @@
const r = await inferTaxon(slabId);
const s = r.suggestions;
const recs: number[] = [];
if (s.species_id) { speciesId = s.species_id; recs.push(s.species_id); }
if (s.species_id) { speciesSel = String(s.species_id); recs.push(s.species_id); }
if (s.wood_category_ids) {
woodCatIds = [...s.wood_category_ids];
if (!primaryCatId) primaryCatId = woodCatIds[0];
recs.push(...s.wood_category_ids);
}
if (s.edge_type_term_id) edgeTypeId = s.edge_type_term_id;
if (s.edge_type_term_id) edgeSel = String(s.edge_type_term_id);
if (s.figure_term_ids) { figureIds = [...s.figure_term_ids]; recs.push(...s.figure_term_ids); }
if (s.feature_tag_ids?.length) {
featTagIds = [...s.feature_tag_ids];
@@ -234,6 +263,9 @@
aiRecommendations = recs;
aiNote = recs.length ? `AI suggested ${recs.length} field${recs.length === 1 ? '' : 's'} — review & edit below.` : 'No confident AI suggestions.';
await commitReview();
// #90: force the <select> DOM to reflect the prefilled values after Svelte's
// own select_option(reset) has run.
syncSelects();
} catch (e) {
aiNote = e instanceof ApiError ? e.message : 'Inference unavailable.';
} finally {
@@ -272,6 +304,22 @@
return arr.includes(id) ? arr.filter((x) => x !== id) : [...arr, id];
}
// #90: keep the native <select> DOM option in sync when the bound value is set
// programmatically (AI prefill, reload hydrate). Svelte 4's update() re-runs
// select_option(select, val) on every val change, which resets el.value to the
// bound value — so a $: sync in the same flush gets overwritten. We defer with
// tick() so the write lands AFTER Svelte's DOM update, guaranteeing the visible
// option matches the bound value. Idempotent: only writes when they differ.
async function syncSelects() {
await tick();
if (speciesEl && speciesEl.value !== speciesSel) speciesEl.value = speciesSel;
if (edgeEl && edgeEl.value !== edgeSel) edgeEl.value = edgeSel;
if (gradeEl && gradeEl.value !== gradeSel) gradeEl.value = gradeSel;
}
let speciesEl: HTMLSelectElement | undefined;
let edgeEl: HTMLSelectElement | undefined;
let gradeEl: HTMLSelectElement | undefined;
onMount(async () => {
try {
const [s, t, set] = await Promise.all([getSlab(slabId), getTaxonomy(), getSettings()]);
@@ -279,6 +327,8 @@
deriveOptions();
hydrate(s, t);
canGenerateContent = !!set.content_llm_enabled;
// #90: sync the <select> DOM to the hydrated values (same race as AI prefill).
syncSelects();
await commitReview();
} catch (e) {
loadError = e instanceof ApiError ? e.message : 'Could not load the slab.';
@@ -333,10 +383,10 @@
<!-- Species (single, AI-assist) -->
<div class="field">
<label class="lbl" for="species">Species <span class="ai">AI assist</span></label>
<select id="species" class="sel" data-testid="species" bind:value={speciesId} on:change={commitReview}>
<select id="species" class="sel" data-testid="species" bind:this={speciesEl} bind:value={speciesSel} on:change={commitReview}>
<option value="">— select —</option>
{#each speciesOpts as o (o.id)}
<option value={o.id} data-ai={o.ai ? 'true' : ''}>{o.name}{o.ai ? ' (AI)' : ''}</option>
<option value={o.id} data-ai={aiRecommendations.includes(o.id) ? 'true' : ''}>{o.name}{aiRecommendations.includes(o.id) ? ' (AI)' : ''}</option>
{/each}
</select>
</div>
@@ -349,12 +399,13 @@
placeholder="Choose wood categories"
testid="wood-cat"
onToggle={(id) => (woodCatIds = toggleMulti(woodCatIds, id))}
onCommit={commitReview}
/>
<!-- Edge type (single, AI-assist) -->
<div class="field">
<label class="lbl" for="edge">Edge type <span class="ai">AI assist</span></label>
<select id="edge" class="sel" data-testid="edge-type" bind:value={edgeTypeId} on:change={commitReview}>
<select id="edge" class="sel" data-testid="edge-type" bind:this={edgeEl} bind:value={edgeSel} on:change={commitReview}>
<option value="">— select —</option>
{#each edgeOpts as o (o.id)}
<option value={o.id}>{o.name}</option>
@@ -372,13 +423,14 @@
placeholder="Choose figure"
testid="figure"
onToggle={(id) => (figureIds = toggleMulti(figureIds, id))}
onCommit={commitReview}
/>
</div>
<!-- Grade (single, owner judgment — NOT AI-assisted per METADATA-40) -->
<div class="field">
<label class="lbl" for="grade">Grade</label>
<select id="grade" class="sel" data-testid="grade" bind:value={gradeId} on:change={commitReview}>
<select id="grade" class="sel" data-testid="grade" bind:this={gradeEl} bind:value={gradeSel} on:change={commitReview}>
<option value="">— select —</option>
{#each gradeOpts as o (o.id)}<option value={o.id}>{o.name}</option>{/each}
</select>
@@ -394,6 +446,7 @@
placeholder="Choose fig-* tags"
testid="fig-tag"
onToggle={(id) => (figTagIds = toggleMulti(figTagIds, id))}
onCommit={commitReview}
/>
</div>
@@ -407,15 +460,14 @@
placeholder="Choose feat-* tags"
testid="feat-tag"
onToggle={(id) => (featTagIds = toggleMulti(featTagIds, id))}
onCommit={commitReview}
/>
</div>
<!-- #88: thickness band is deterministic from thickness_in — display-only, no owner pick. -->
<div class="field">
<label class="lbl" for="thickness">Thickness band</label>
<select id="thickness" class="sel" data-testid="thickness-band" bind:value={thicknessId} on:change={commitReview}>
<option value="">— select —</option>
{#each thicknessOpts as o (o.id)}<option value={o.id}>{o.name}</option>{/each}
</select>
<label class="lbl" for="thickness">Thickness band <span class="sug">from thickness</span></label>
<div id="thickness" class="ro" data-testid="thickness-band">{thicknessDisplay}</div>
</div>
<div class="field">
<label class="lbl" for="moisture">Moisture</label>
@@ -504,6 +556,8 @@
.lbl { display: block; font-size: .8rem; color: #bbb; margin: 0 0 4px; }
.lbl .ai { margin-left: 6px; font-size: .58rem; font-weight: 700; padding: 1px 5px; border-radius: 2px; background: #00e5ff; color: #002; }
.inp, .sel { width: 100%; padding: 9px; background: #1d1d1d; border: 1px solid #444; border-radius: 3px; color: #eee; font-size: .9rem; text-align: left; }
/* #88 read-only thickness band: same chrome as .sel but non-interactive. */
.ro { width: 100%; padding: 9px; background: #141414; border: 1px solid #333; border-radius: 3px; color: #9ad0d8; font-size: .9rem; text-align: left; }
.sel { appearance: none; background-image: linear-gradient(45deg,transparent 50%,#777 50%),linear-gradient(135deg,#777 50%,transparent 50%); background-position: calc(100% - 16px) 55%,calc(100% - 11px) 55%; background-size: 5px 5px; background-repeat: no-repeat; }
.ta { resize: vertical; }
.field { margin-bottom: 12px; }
@@ -0,0 +1,57 @@
import assert from 'node:assert/strict';
import { test } from 'node:test';
import { deriveThicknessTermId, type ThicknessBand } from './thicknessBand';
// Synced-Woo-shaped bands (id + name). The e2e fixture uses a single "2-4 in" band.
const BANDS: ThicknessBand[] = [
{ id: 39, name: '2-4 in' },
];
test('TV-9 / QUALITY-95: 1 in rounds up into the enclosing synced band', () => {
const bands: ThicknessBand[] = [
{ id: 10, name: '1-2 in' },
{ id: 39, name: '2-4 in' },
];
assert.equal(deriveThicknessTermId(1, bands), 10);
assert.equal(deriveThicknessTermId(1.5, bands), 10);
// 2.0 is exactly the upper bound of the 1-2 band → belongs to it.
assert.equal(deriveThicknessTermId(2.0, bands), 10);
// 2.01 falls into 2-4.
assert.equal(deriveThicknessTermId(2.25, bands), 39);
assert.equal(deriveThicknessTermId(4, bands), 39);
});
test('e2e fixture: thickness_in 2 → the single "2-4 in" band (id 39)', () => {
assert.equal(deriveThicknessTermId(2, BANDS), 39);
});
test('above every band → empty (no invented band; upstream thickness fix resolves it)', () => {
assert.equal(deriveThicknessTermId(5, BANDS), '');
});
test('no thickness / non-finite → empty', () => {
assert.equal(deriveThicknessTermId(null, BANDS), '');
assert.equal(deriveThicknessTermId(undefined, BANDS), '');
assert.equal(deriveThicknessTermId(0, BANDS), '');
assert.equal(deriveThicknessTermId(NaN, BANDS), '');
assert.equal(deriveThicknessTermId(2, []), '');
});
test('band names are parsed from the synced names, never hardcoded (order-independent)', () => {
// Reverse the array: the derivation must still pick the right band by bound.
const bands: ThicknessBand[] = [
{ id: 39, name: '2-4 in' },
{ id: 10, name: '1-2 in' },
];
assert.equal(deriveThicknessTermId(1.5, bands), 10);
assert.equal(deriveThicknessTermId(3, bands), 39);
});
test('malformed band names are skipped without throwing', () => {
const bands: ThicknessBand[] = [
{ id: 1, name: 'N/A' },
{ id: 2, name: '4-2 in' }, // inverted, unusable
{ id: 39, name: '2-4 in' },
];
assert.equal(deriveThicknessTermId(3, bands), 39);
});
@@ -0,0 +1,58 @@
/**
* Deterministic thickness-band derivation (TV-9 / QUALITY-95, METADATA-15/25).
*
* The Woo "Thickness" attribute is a set of named bands (e.g. "2-4 in"). The
* owner enters a numeric thickness in 1/8-inch steps; the band is derived by
* rounding that thickness UP into the band whose numeric upper bound first
* encloses it. The Review UI never exposes a band picker — the band always
* follows from thickness_in (issue #88).
*
* Pure function so it is unit-testable without a browser/backend.
*/
export interface ThicknessBand {
id: number;
name: string;
}
/** Parse the leading numeric bounds ("2-4 in" → {min:2,max:4}). Returns null if the name
* carries no usable numeric upper bound (defensive — a band we can't place is skipped). */
function parseBounds(name: string): { min: number; max: number } | null {
// Match the first two numbers in the name, in order (handles "2-4 in", "1/8", "12in+").
const nums = name.match(/\d+(?:[./]\d+)?/g);
if (!nums) return null;
const asNum = (s: string) => Number(s);
const min = asNum(nums[0]);
const max = nums.length >= 2 ? asNum(nums[1]) : min;
if (!Number.isFinite(min) || !Number.isFinite(max)) return null;
if (max < min) return null; // malformed "4-2"; not a usable band
return { min, max };
}
/**
* Round `thicknessIn` (inches, 1/8" steps) up into the synced Woo thickness band.
* Returns the term id, or '' when no band encloses the value (no thickness known,
* empty synced set, or a value above every band — all leave the field empty so the
* owner's upstream fix, not a wrong band, resolves it).
*/
export function deriveThicknessTermId(
thicknessIn: number | null | undefined,
bands: ThicknessBand[],
): number | '' {
if (typeof thicknessIn !== 'number' || !Number.isFinite(thicknessIn) || thicknessIn <= 0) {
return '';
}
// Sort bands by upper bound so "first enclosing max" is well-defined regardless of
// the order Woo returned them in.
const ranked = bands
.map((b) => ({ b, bounds: parseBounds(b.name) }))
.filter((x): x is { b: ThicknessBand; bounds: { min: number; max: number } } => x.bounds !== null)
.sort((a, z) => a.bounds.max - z.bounds.max);
for (const { b, bounds } of ranked) {
// A value exactly on the band's max belongs to that band; above it, fall through.
if (thicknessIn <= bounds.max) {
return b.id;
}
}
return '';
}