diff --git a/.changeset/8632-malformed-picklist-option-loud.md b/.changeset/8632-malformed-picklist-option-loud.md new file mode 100644 index 0000000000..7cd08db8df --- /dev/null +++ b/.changeset/8632-malformed-picklist-option-loud.md @@ -0,0 +1,47 @@ +--- +'@object-ui/app-shell': minor +--- + +Report a malformed picklist option in the field designer instead of showing it as a +blank row and deleting it (objectui#8632). + +**The half that did the damage is the deletion.** `ObjectFieldInspector`'s +`readOptions` opened with `value: String(o?.value ?? '')`, so every option entry it +could not read arrived as `value: ''` — two empty input boxes. `OptionsEditor` persists +only rows with a non-empty `value`, so those entries were then written out of the +document. Measured on `options: ['draft','open','closed']`: the list rendered as three +blank rows, and **one click on "Add value", with nothing typed, wrote `options: []`**. +An author who opened a picklist, saw an empty-looking option list, and clicked the +obvious button lost three authored options they had never been shown, with nothing on +screen attributing the loss to anything they did. + +**"Malformed" was two families, not one shape.** Beyond the entries that collapsed to a +blank row (a bare string, `null`, a number, a boolean, `{}`, an option with no `value`, +an authored empty `value`, a nested array), a second family was silently **rewritten** +and never looked wrong: `{ value: 5 }` was written back as `"5"`, `{ value: ['alpha'] }` +as `"alpha"`, `{ value: { a: 1 } }` as `"[object Object]"`, a non-string `label` as +`label: ''`, and a non-string `color` was dropped from the document. Both families are +now covered by one rule. + +**What changed.** The reader is now strict — the `String()` coercion is gone rather than +widened, per AGENTS.md #0.1 — and classifies each authored entry. An entry this editor +cannot represent faithfully gets a marked row of its own naming the reason, showing the +authored entry verbatim, and carrying the same reorder/remove controls as any other row; +it is written back **byte for byte as authored** on every commit. Removing it stays +available and stays deliberate. + +**This narrows what the designer will save.** A document with a malformed option used to +become saveable because the designer silently deleted the offending entries; it no longer +does. The entry is preserved, so the draft keeps failing `FieldSchema` until the author +repairs or removes it — which is the reported state rather than a silent repair. The +escape path is one click on the row's Remove button, and it reproduces exactly the old +outcome with the author choosing it. + +Well-formed option sets are untouched: they render and commit key for key as before, +including the `default` / `visibleWhen` carrier (objectui#7540), the `label: ''` emitted +for an option with no `label` key (objectui#7014), and the editor's own blank trailing +row, which is still filtered on commit. + +Two new strings land in the designer's own `en` / `zh` tables — the metadata-admin +console owns its strings in `views/metadata-admin/i18n.ts` and is deliberately outside +the ten locale packs (`packages/i18n/README.md`, "Scope — the `engine.*` carve-out"). diff --git a/packages/app-shell/src/views/metadata-admin/i18n.ts b/packages/app-shell/src/views/metadata-admin/i18n.ts index 6cbfed5663..c5d701b870 100644 --- a/packages/app-shell/src/views/metadata-admin/i18n.ts +++ b/packages/app-shell/src/views/metadata-admin/i18n.ts @@ -1392,6 +1392,13 @@ const ENGINE_STRINGS_EN: Record = { 'designer.field.noGroup': '— No group —', 'designer.field.picklistValues': 'Picklist values', 'designer.field.noValues': 'No values yet.', + 'designer.field.optMalformed': 'This option cannot be edited here', + 'designer.field.optMalformed.notAnObject': 'It is not an option object.', + 'designer.field.optMalformed.valueNotText': 'Its `value` is missing or is not text.', + 'designer.field.optMalformed.valueEmpty': 'Its `value` is empty.', + 'designer.field.optMalformed.labelNotText': 'Its `label` is not text.', + 'designer.field.optMalformed.colorNotText': 'Its `color` is not text.', + 'designer.field.optMalformedHint': 'It is kept exactly as authored. Repair it in the JSON source, or remove it here.', 'designer.field.addValue': 'Add value', 'designer.field.optValue': 'value', 'designer.field.optLabel': 'Label', @@ -3336,6 +3343,13 @@ const ENGINE_STRINGS_ZH: Record = { 'designer.field.noGroup': '— 无分组 —', 'designer.field.picklistValues': '选项值', 'designer.field.noValues': '暂无选项值。', + 'designer.field.optMalformed': '此选项无法在这里编辑', + 'designer.field.optMalformed.notAnObject': '它不是一个选项对象。', + 'designer.field.optMalformed.valueNotText': '它的 `value` 缺失或不是文本。', + 'designer.field.optMalformed.valueEmpty': '它的 `value` 为空。', + 'designer.field.optMalformed.labelNotText': '它的 `label` 不是文本。', + 'designer.field.optMalformed.colorNotText': '它的 `color` 不是文本。', + 'designer.field.optMalformedHint': '它按原样保留。请在 JSON 源码中修复,或在此处删除。', 'designer.field.addValue': '添加选项', 'designer.field.optValue': '值', 'designer.field.optLabel': '显示名', diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.malformedOptions.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.malformedOptions.test.tsx new file mode 100644 index 0000000000..b7332eaa13 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.malformedOptions.test.tsx @@ -0,0 +1,326 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Pins that a picklist option this designer CANNOT represent is reported on + * screen and survives the next edit, instead of rendering blank and then being + * deleted (objectui#8632). + * + * ## The half that did the damage is the DELETION, not the blank row + * + * `readOptions` used to open with `value: String(o?.value ?? '')`, so every + * entry it could not read became `value: ''` — a row with two empty boxes. + * `OptionsEditor.commit` persists only rows with a non-empty `value`. So the + * author saw nothing wrong AND lost the content on the next touch of the option + * editor. Measured on the unfixed reader, `options: ['draft','open','closed']`: + * + * render -> 3 rows, values ["","",""], labels ["","",""] + * click "Add value" (no typing) -> options: [] <- three options gone + * one reorder click -> options: [] + * type "alpha" into row 0 -> options: [{value:"alpha",label:""}] + * + * ⚠️ CORRECTION to the card's own account, measured on the unfixed reader and + * pinned below so it is not re-asserted: an edit to an UNRELATED control on the + * same field — Description, Label, Required — never rewrote `options` at all. + * `patchDef` spreads `def` and patches only the keys it is handed. The trigger + * is any interaction INSIDE the option editor, which is exactly where an author + * looking at an empty-looking option list goes next. The reachability is worse + * than the card claimed, not better: it costs one click, before a character is + * typed. + * + * ## "Malformed" was never one shape — it is two families + * + * Both measured on the unfixed reader. A repair that caught only the first + * would have looked complete: + * + * COLLAPSED to an empty row, then deleted — a bare string, `null`, `5`, + * `true`, `{}`, `{ label } with no value`, `{ value: '' }`, `{ value: null }`, + * a nested array. + * + * SILENTLY REWRITTEN into a different document, never deleted, never visibly + * wrong — `{ value: 5 }` -> `"5"`, `{ value: true }` -> `"true"`, + * `{ value: ['alpha'] }` -> `"alpha"` (indistinguishable on screen from a + * well-formed option), `{ value: { a: 1 } }` -> `"[object Object]"`, + * `{ label: 5 }` -> `label: ''`, `{ color: 16711680 }` -> `color` dropped. + * + * The second family is `String()` coercion — AGENTS.md #0.1 consumer-side + * tolerance — which is why the repair REMOVES that expression rather than + * widening it further. + * + * ## Two boundaries, both prior rulings rather than oversights + * + * • `{ value: 'alpha' }` with NO `label` key still commits `label: ''`. That + * is the objectui#7014 Q2 ruling and it is pinned here as a control. + * • `{ value: 'a', label: 'A' }` — representable here, rejected by the spec + * (`too_small@[value]`, the two-character minimum). It stays an ordinary + * editable row; the draft validator names it. This reader reports only what + * it cannot SHOW. + * + * Every case is refusal-shaped: it asserts what the WRITTEN document carries, + * and the preserved entries are put back to `FieldSchema` to show that + * preserving them is not a claim that they are valid — the gate still refuses + * the document, which is the whole point of not silently repairing it. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, cleanup, within } from '@testing-library/react'; +import { SelectOptionSchema, FieldSchema } from '@objectstack/spec/data'; + +vi.mock('../useMetadata', () => ({ + useMetadataClient: () => ({ + list: vi.fn().mockResolvedValue([]), + listDrafts: vi.fn().mockResolvedValue([]), + }), +})); + +vi.mock('../previews/useObjectFields', () => ({ + useObjectFields: () => ({ fields: [], loading: false, error: null }), +})); + +import { ObjectFieldInspector } from './ObjectFieldInspector'; + +afterEach(cleanup); + +type Def = Record; + +function renderField(options: unknown[]) { + const onPatch = vi.fn(); + render( + , + ); + /** The option list the designer would persist for `status` after the last edit. */ + const savedOptions = (): unknown[] => { + const call = onPatch.mock.calls.at(-1); + if (!call) throw new Error('the editor never called onPatch — no round trip to measure'); + const fields = call[0].fields as Record; + return ((fields.status as { options?: unknown[] }).options ?? []) as unknown[]; + }; + return { onPatch, savedOptions }; +} + +/** The rows the editor offers as editable options (a malformed row has no inputs). */ +const editableRows = () => screen.queryAllByPlaceholderText('value') as HTMLInputElement[]; +/** The rows the editor refuses to represent. */ +const malformedRows = () => screen.queryAllByTestId('option-malformed'); + +/** Add a value: one click, nothing typed. The cheapest trigger there is. */ +const clickAddValue = () => fireEvent.click(screen.getByText('Add value')); + +/** The structural slice of a Zod schema this file needs -- no `any` (AGENTS.md #6). */ +type SpecIssue = { code: string; path: ReadonlyArray }; +type SpecSchema = { + safeParse: (value: unknown) => { success: boolean; error?: { issues: SpecIssue[] } }; +}; +const rejectionsOf = (schema: SpecSchema, doc: unknown): string[] => { + const r = schema.safeParse(doc); + return r.success ? [] : (r.error?.issues ?? []).map((i) => `${i.code}@[${i.path.join('.')}]`); +}; + +describe('ObjectFieldInspector · the destructive half — a malformed option is no longer deleted (objectui#8632)', () => { + it('one click on "Add value", nothing typed, no longer erases three authored options', () => { + const { savedOptions } = renderField(['draft', 'open', 'closed']); + + // The rows are visible AS malformed before anything is touched, so the + // author is not clicking blind. + expect(malformedRows()).toHaveLength(3); + + clickAddValue(); + + // THE PIN. Before the repair this read `[]`. + expect(savedOptions()).toEqual(['draft', 'open', 'closed']); + // LIT CONTROL: the click really reached the writer and really added a row, + // so the assertion above measures a round trip rather than a render that + // never called `onChange`. The blank row the click created is filtered on + // commit (that filter is unchanged), which is why the written list is three + // and the on-screen editable rows are one. + expect(editableRows()).toHaveLength(1); + }); + + it('a reorder click preserves them — in the new order, and still verbatim', () => { + const { savedOptions } = renderField(['draft', 'open', 'closed']); + fireEvent.click(screen.getAllByLabelText('Move down')[0]); + expect(savedOptions()).toEqual(['open', 'draft', 'closed']); + }); + + it('typing into a WELL-FORMED row does not erase its malformed siblings', () => { + const { savedOptions } = renderField(['draft', { value: 'open', label: 'Open' }, 'closed']); + fireEvent.change(screen.getByPlaceholderText('Label'), { target: { value: 'Open now' } }); + expect(savedOptions()).toEqual(['draft', { value: 'open', label: 'Open now' }, 'closed']); + }); + + it('preserving it is NOT a claim that it is valid — the document still fails the contract', () => { + const { savedOptions } = renderField(['draft']); + clickAddValue(); + const written = savedOptions(); + expect(written).toEqual(['draft']); + // The gate keeps refusing the field, which is what makes the preservation + // honest: the author is told, the content is kept, and nothing pretends the + // draft is saveable until they repair it. + expect(rejectionsOf(SelectOptionSchema as SpecSchema, written[0]).length).toBeGreaterThan(0); + expect( + rejectionsOf(FieldSchema as SpecSchema, { name: 'status', type: 'select', label: 'Status', options: written }) + .length, + ).toBeGreaterThan(0); + }); + + it('the escape path is one deliberate click — Remove drops it, and only it', () => { + // ZONE 2 D: the author is never trapped. Removing the row reproduces + // exactly the old outcome, with the author choosing it. + const { savedOptions } = renderField(['draft', { value: 'open', label: 'Open' }]); + const row = malformedRows()[0]; + fireEvent.click(within(row).getByLabelText('Remove')); + expect(savedOptions()).toEqual([{ value: 'open', label: 'Open' }]); + expect(malformedRows()).toHaveLength(0); + }); +}); + +describe('ObjectFieldInspector · the invisibility half — the row says what is wrong', () => { + it('reports the reason and shows the authored entry verbatim, instead of two blank boxes', () => { + renderField(['draft']); + const row = malformedRows()[0]; + expect(row).toBeTruthy(); + expect(row.getAttribute('role')).toBe('alert'); + expect(within(row).getByText('This option cannot be edited here')).toBeTruthy(); + expect(within(row).getByText('It is not an option object.')).toBeTruthy(); + // The authored entry, so the author can recognise their own content. + expect(within(row).getByText('"draft"')).toBeTruthy(); + // ...and no blank editable row impersonating it. + expect(editableRows()).toHaveLength(0); + }); + + it('names the RIGHT reason per shape — a class report, not one message for everything', () => { + const cases: Array<[unknown, string]> = [ + [{ label: 'Draft' }, 'Its `value` is missing or is not text.'], + [{ value: '', label: 'Draft' }, 'Its `value` is empty.'], + [{ value: 'draft', label: 5 }, 'Its `label` is not text.'], + [{ value: 'draft', label: 'Draft', color: 16711680 }, 'Its `color` is not text.'], + ]; + for (const [authored, expected] of cases) { + cleanup(); + renderField([authored]); + expect(within(malformedRows()[0]).getByText(expected)).toBeTruthy(); + } + }); +}); + +describe('ObjectFieldInspector · the census — both families of malformed option', () => { + // Family 1 was collapsed to a blank row and DELETED. Family 2 was silently + // REWRITTEN and never looked wrong. Every entry here is asserted the same + // way, which is the point: one rule replaced a list of shapes. + const family1: Array<[string, unknown]> = [ + ['a bare string', 'draft'], + ['null', null], + ['a number', 5], + ['a boolean', true], + ['an empty object', {}], + ['an option with no value', { label: 'Draft' }], + ['an authored empty value', { value: '', label: 'Draft' }], + ['a null value', { value: null, label: 'Draft' }], + ['a nested array', ['draft', 'open']], + ]; + const family2: Array<[string, unknown]> = [ + ['a numeric value', { value: 5, label: 'Five' }], + ['a boolean value', { value: true, label: 'Yes' }], + ['an object value', { value: { a: 1 }, label: 'Obj' }], + ['an array value', { value: ['alpha'], label: 'Arr' }], + ['a non-string label', { value: 'alpha', label: 5 }], + ['a non-string color', { value: 'alpha', label: 'Alpha', color: 16711680 }], + ]; + + for (const [family, cases] of [ + ['family 1 (was rendered blank, then deleted)', family1] as const, + ['family 2 (was silently rewritten into a different document)', family2] as const, + ]) { + for (const [name, authored] of cases) { + it(`${family}: ${name} is reported and survives a sibling edit verbatim`, () => { + const { savedOptions } = renderField([authored, { value: 'zzz9', label: 'Sentinel' }]); + expect(malformedRows()).toHaveLength(1); + // Edit the OTHER row -- the trigger that used to destroy this one. + fireEvent.change(screen.getByPlaceholderText('Label'), { target: { value: 'Sentinel II' } }); + expect(savedOptions()).toEqual([authored, { value: 'zzz9', label: 'Sentinel II' }]); + }); + } + } +}); + +describe('ObjectFieldInspector · the control — a well-formed option set is untouched', () => { + it('renders and commits exactly as before, key for key', () => { + const { savedOptions } = renderField([ + { value: 'alpha', label: 'Alpha', color: '#ff0000' }, + { value: 'beta', label: 'Beta', default: true, visibleWhen: 'x > 1' }, + ]); + // No refusal anywhere on a clean document. + expect(malformedRows()).toHaveLength(0); + expect(editableRows()).toHaveLength(2); + + fireEvent.change(screen.getAllByPlaceholderText('Label')[0], { target: { value: 'Alpha II' } }); + + // Byte for byte the projection this editor has always written: `rest` + // spread first (objectui#7540), `label` always emitted, `color` only when + // truthy. + expect(savedOptions()).toEqual([ + { value: 'alpha', label: 'Alpha II', color: '#ff0000' }, + { default: true, visibleWhen: 'x > 1', value: 'beta', label: 'Beta' }, + ]); + for (const written of savedOptions()) { + expect(rejectionsOf(SelectOptionSchema as SpecSchema, written)).toEqual([]); + } + }); + + it("the editor's own blank trailing row is still filtered on commit", () => { + // This is what the `value.trim() !== ''` filter was written for, and it is + // deliberately unchanged. An AUTHORED empty value is a different fact and + // is covered by the census above. + const { savedOptions } = renderField([{ value: 'alpha', label: 'Alpha' }]); + clickAddValue(); + expect(editableRows()).toHaveLength(2); + expect(savedOptions()).toEqual([{ value: 'alpha', label: 'Alpha' }]); + }); + + it('an option with NO label key still commits `label: ""` (objectui#7014 Q2, preserved)', () => { + const { savedOptions } = renderField([{ value: 'alpha' }, { value: 'beta', label: 'Beta' }]); + expect(malformedRows()).toHaveLength(0); + fireEvent.change(screen.getAllByPlaceholderText('Label')[1], { target: { value: 'Beta II' } }); + expect(savedOptions()).toEqual([ + { value: 'alpha', label: '' }, + { value: 'beta', label: 'Beta II' }, + ]); + }); + + it('a value the SPEC rejects but this editor can show stays an ordinary editable row', () => { + const { savedOptions } = renderField([{ value: 'a', label: 'A' }]); + expect(malformedRows()).toHaveLength(0); + expect(editableRows()).toHaveLength(1); + fireEvent.change(screen.getByPlaceholderText('Label'), { target: { value: 'A II' } }); + expect(savedOptions()).toEqual([{ value: 'a', label: 'A II' }]); + // The reader does not report it; the contract does, and that division is + // the reason this row is left alone. + expect(rejectionsOf(SelectOptionSchema as SpecSchema, { value: 'a', label: 'A II' })).toEqual([ + 'too_small@[value]', + ]); + }); +}); + +describe('ObjectFieldInspector · the premise correction — what an unrelated edit actually does', () => { + it('an edit to another control on the same field never rewrites `options`', () => { + // Measured identically on the unfixed reader. `patchDef` spreads `def` and + // patches only the keys handed to it, so `options` travels untouched. The + // card and its PM note both described the deletion as reachable this way; + // it is not, and pinning that keeps the next reader from re-asserting it. + const { savedOptions } = renderField(['draft', 'open', 'closed']); + const description = document.querySelectorAll('textarea')[0]; + fireEvent.change(description, { target: { value: 'the field description' } }); + fireEvent.blur(description); + expect(savedOptions()).toEqual(['draft', 'open', 'closed']); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx index e2569f375b..4a98dfb608 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx @@ -17,7 +17,10 @@ * spec-rejected keys `object-fields-io` strips on read * (`RETIRED_FIELD_KEYS`). The same holds one level down for a picklist * option: `readOptions` carries the keys the option editor has no control - * for and `patchOptions` writes them back (objectui#7540). + * for and `patchOptions` writes them back (objectui#7540), and an option the + * editor cannot represent AT ALL is reported on its own row and written back + * verbatim rather than shown blank and dropped (objectui#8632 — see + * `classifyOption`). * * There is deliberately no `Indexed` control here (objectui#4644): the * spec has no field-level index flag, `FieldSchema.safeParse` rejects @@ -46,7 +49,7 @@ import { moveArray, } from './_shared.js'; import { Button, Input, Label, Badge } from '@object-ui/components'; -import { Plus, X, ArrowUp, ArrowDown, Copy } from 'lucide-react'; +import { Plus, X, ArrowUp, ArrowDown, Copy, AlertTriangle } from 'lucide-react'; import { InspectorComboField, type InspectorComboOption } from './InspectorComboField.js'; import { useObjectFields } from '../previews/useObjectFields.js'; import { @@ -102,6 +105,125 @@ interface Option { /* ─────────────── Helpers ─────────────── */ +/** + * Why an option this editor cannot represent is REPORTED rather than coerced + * (objectui#8632). + * + * `MalformedOption` is the other half of `Option`: one authored entry of + * `def.options` that this editor has no faithful representation for. It is not + * an error state of a row — it is a row of its own kind, and the two travel + * together in `OptionRow` so a malformed entry keeps its POSITION in the list. + * + * `raw` is the authored entry verbatim, and it is what `patchOptions` writes + * back. That is the whole repair: the reader stops inventing a value it was + * never given, and the writer stops deleting what it cannot read. + */ +type MalformedReason = + /** The entry is not an option object at all — a bare string, `null`, a number, an array. */ + | 'not-an-object' + /** `value` is absent, or present with a non-string type. */ + | 'value-not-text' + /** `value` is a string but blank — authored, so NOT this editor's own trailing blank row. */ + | 'value-empty' + /** `label` is present with a non-string type. */ + | 'label-not-text' + /** `color` is present with a non-string type. */ + | 'color-not-text'; + +interface MalformedOption { + /** The authored entry, untouched. Written back byte-for-byte on commit. */ + raw: unknown; + reason: MalformedReason; +} + +/** + * One row of the option editor: either an option it owns, or an entry it + * refuses to represent. `kind` is an explicit tag rather than a `'malformed' in + * row` test so every consumer has to answer the question. + */ +type OptionRow = + | { kind: 'option'; option: Option } + | { kind: 'malformed'; malformed: MalformedOption }; + +/** + * Why the reader stays STRICT — the ruling this function is the subject of. + * + * This reader used to open with `value: String(o?.value ?? '')`. That single + * expression is consumer-side tolerance (AGENTS.md #0.1) and it produced BOTH + * halves of objectui#8632, in two different directions, each measured on the + * unfixed reader: + * + * • It manufactured an empty value for every entry it could not read — a bare + * string, `null`, `5`, `true`, `{}`, `{ label }` with no `value`. The author + * saw a BLANK row: three authored options rendered as three empty boxes. + * Then `OptionsEditor.commit` — which persists only rows with a non-empty + * `value` — DELETED them. Measured trigger: one click on "Add value", with + * nothing typed, wrote `options: []` over three authored options. + * • It silently REWROTE every entry whose `value` it could stringify into + * something else: `{ value: 5 }` was written back as `"5"`, `{ value: true }` + * as `"true"`, `{ value: ['alpha'] }` as `"alpha"` (indistinguishable on + * screen from a well-formed option), `{ value: { a: 1 } }` as + * `"[object Object]"`. Same for the other two keys this editor owns: + * `label: 5` was written back as `label: ''`, and a non-string `color` was + * dropped from the document entirely. + * + * A repair that only caught the first family would have looked complete and + * left the second one deleting authored content exactly as before, so the rule + * here is one rule, not a list of shapes: an entry this editor cannot represent + * FAITHFULLY is not represented. It is reported on its own row and carried + * through verbatim, and the author removes or repairs it deliberately. + * + * Two boundaries this classifier deliberately does NOT cross, because both are + * prior rulings in this file rather than oversights: + * + * • A MISSING `label` is not malformed. `patchOptions` emits `label: ''` for + * it, which is the objectui#7014 Q2 ruling: `''` is what the Label box has + * been showing the author all along, and the spec accepts it. A present but + * non-string `label` is a different fact — there are authored bytes being + * destroyed — and that one IS malformed. + * • A `value` this editor can represent but the SPEC rejects (`'a'` — the + * select option's two-character minimum) stays an ordinary editable row. + * The author can see it and fix it in place, and the draft validator is the + * surface that names it. This reader reports only what it cannot show. + */ +function classifyOption(raw: unknown): OptionRow { + if (!raw || typeof raw !== 'object' || Array.isArray(raw)) { + return { kind: 'malformed', malformed: { raw, reason: 'not-an-object' } }; + } + const o = raw as Record; + if (typeof o.value !== 'string') { + return { kind: 'malformed', malformed: { raw, reason: 'value-not-text' } }; + } + if (o.value.trim() === '') { + return { kind: 'malformed', malformed: { raw, reason: 'value-empty' } }; + } + if (o.label !== undefined && typeof o.label !== 'string') { + return { kind: 'malformed', malformed: { raw, reason: 'label-not-text' } }; + } + if (o.color !== undefined && typeof o.color !== 'string') { + return { kind: 'malformed', malformed: { raw, reason: 'color-not-text' } }; + } + // Representable. Everything below is the projection this reader has always + // produced for a well-formed option, unchanged — see the round-trip control + // in `ObjectFieldInspector.malformedOptions.test.tsx`. + const rest: Record = { ...o }; + // The keys the editor owns live in their own named slots. Removing them + // here is what keeps `patchOptions` from having two sources for one key. + delete rest.value; + delete rest.label; + delete rest.color; + const option: Option = { + value: o.value, + label: typeof o.label === 'string' ? o.label : undefined, + color: typeof o.color === 'string' ? o.color : undefined, + }; + // Only attach the carrier when there is something to carry, so an option + // with nothing extra stays byte-identical to what this reader used to + // produce. + if (Object.keys(rest).length > 0) option.rest = rest; + return { kind: 'option', option }; +} + /** * Read `def.options` into editor rows, keeping the WHOLE authored option. * @@ -114,6 +236,11 @@ interface Option { * only carry what this function handed it, and this function handed it three * keys. The reader is where `default` and `visibleWhen` disappeared. * + * It is the loss site a second time for objectui#8632, and in the same shape: + * an entry it could not read was handed on as an empty row and deleted by the + * writer. Both repairs are the same move — hand on WHAT WAS AUTHORED — which is + * why the classifier above lives here and not in `OptionsEditor`. + * * The shape mirrors the field-level door one level up: `readFields` in * `previews/object-fields-io.ts` preserves unknown keys on a field definition * the same way (its `...rest`), stripping only the named keys a shipped build @@ -122,31 +249,15 @@ interface Option { * only ever written `value` / `label` / `color`, so no key it authored can * come back as one the spec rejects. */ -function readOptions(def: Record): Option[] { +function readOptions(def: Record): OptionRow[] { const raw = def.options; if (!Array.isArray(raw)) return []; - return raw.map((o: any) => { - // A non-object entry (e.g. a bare string in a hand-written `options: []`) - // has no keys to carry — it already collapses to an empty `value` below, - // and an empty-valued row is dropped on commit. - const rest: Record = - o && typeof o === 'object' && !Array.isArray(o) ? { ...o } : {}; - // The keys the editor owns live in their own named slots. Removing them - // here is what keeps `patchOptions` from having two sources for one key. - delete rest.value; - delete rest.label; - delete rest.color; - const row: Option = { - value: String(o?.value ?? ''), - label: typeof o?.label === 'string' ? o.label : undefined, - color: typeof o?.color === 'string' ? o.color : undefined, - }; - // Only attach the carrier when there is something to carry, so an option - // with nothing extra stays byte-identical to what this reader used to - // produce. - if (Object.keys(rest).length > 0) row.rest = rest; - return row; - }); + return raw.map(classifyOption); +} + +/** The rows the rest of the inspector can offer as real choices. */ +function representableOptions(rows: OptionRow[]): Option[] { + return rows.flatMap((row) => (row.kind === 'option' ? [row.option] : [])); } function isPicklist(type: string): boolean { @@ -518,9 +629,19 @@ export function ObjectFieldInspector({ /* ─── Option editor ─── */ - const options = readOptions(def); - const patchOptions = (next: Option[]) => { - const clean = next.map((o) => { + const optionRows = readOptions(def); + const options = representableOptions(optionRows); + const patchOptions = (next: OptionRow[]) => { + const clean = next.map((row) => { + // An entry this editor refuses to represent is written back EXACTLY as it + // was authored (objectui#8632). It was never shown, so there is nothing + // the author could have meant by "keep it" or "drop it" — and the writer + // that used to drop it did so on the strength of an empty `value` this + // reader had invented. Carrying it verbatim is what makes the inline + // report on its row honest: the row says "this is what is in your + // document", and the document still says it after the next edit. + if (row.kind === 'malformed') return row.malformed.raw; + const o = row.option; // `label` is REQUIRED by the spec's select option, and an EMPTY label is // a document it accepts: measured on `@objectstack/spec` 17.2.0, // `{ value: 'alpha', label: '' }` -> ACCEPT, while `{ value: 'alpha' }` @@ -533,9 +654,11 @@ export function ObjectFieldInspector({ // // So emit what the author holds, empty string included. `??` rather than // `||` is load-bearing: `||` is the same truthiness bug spelled shorter. - // The `?? ''` arm also covers the option that arrived without a usable - // label at all (`readOptions` maps a missing or non-string `label` to - // `undefined`) -- there is no legal document that omits the key, and '' + // The `?? ''` arm also covers the option that arrived without a `label` + // key at all (`readOptions` maps a MISSING label to `undefined`; since + // objectui#8632 a PRESENT non-string label is a malformed row instead, + // and never reaches here) -- there is no legal document that omits the + // key, and '' // is precisely what the Label input has been showing the author for that // option all along (`value={o.label ?? ''}`), so this emits what they // see rather than inventing content. @@ -696,7 +819,7 @@ export function ObjectFieldInspector({ {isPicklist(type) && ( = { + 'not-an-object': 'designer.field.optMalformed.notAnObject', + 'value-not-text': 'designer.field.optMalformed.valueNotText', + 'value-empty': 'designer.field.optMalformed.valueEmpty', + 'label-not-text': 'designer.field.optMalformed.labelNotText', + 'color-not-text': 'designer.field.optMalformed.colorNotText', +}; + +/** The authored entry, rendered for a human, with a ceiling so one bad row cannot own the panel. */ +function describeMalformed(raw: unknown): string { + let text: string; + try { + text = JSON.stringify(raw) ?? String(raw); + } catch { + // A document that came off the wire cannot be cyclic, but this reader is + // handed whatever the draft holds and must not be the thing that throws. + text = String(raw); + } + return text.length > 120 ? `${text.slice(0, 119)}…` : text; +} + function OptionsEditor({ - options, + rows: incoming, onChange, disabled, locale, }: { - options: Option[]; - onChange: (next: Option[]) => void; + rows: OptionRow[]; + onChange: (next: OptionRow[]) => void; disabled?: boolean; locale?: string; }) { @@ -1100,22 +1245,71 @@ function OptionsEditor({ // only PERSIST rows whose `value` is non-empty — otherwise the blank row // fails the spec identifier rule ("System identifier must be at least 2 // characters") and shows a confusing error mid-edit. The editor is remounted - // per field (key={entry.name}), so seeding from `options` once is correct. - const [rows, setRows] = React.useState( - () => (options.length > 0 ? options : [{ value: '', label: '' }]), + // per field (key={entry.name}), so seeding from `incoming` once is correct. + // + // ⚠️ That `value`-is-empty filter is HALF of objectui#8632, and the half that + // did the damage. It is correct for the row it was written for — the trailing + // blank this editor creates — and it was catastrophic for an AUTHORED entry + // the old reader had collapsed into the same shape: one click on "Add value", + // with nothing typed, wrote `options: []` over three authored options. The + // fix is upstream, in `classifyOption`: an authored entry never arrives here + // wearing the blank row's shape any more, so the filter below can go on + // meaning exactly what it says. It is deliberately unchanged. + const [rows, setRows] = React.useState( + () => (incoming.length > 0 ? incoming : [{ kind: 'option', option: { value: '', label: '' } }]), ); - const commit = (next: Option[]) => { + const commit = (next: OptionRow[]) => { setRows(next); - onChange(next.filter((o) => o.value.trim() !== '')); + onChange(next.filter((r) => r.kind === 'malformed' || r.option.value.trim() !== '')); }; const update = (i: number, patch: Partial