From 7001725f4afdc6f50469082135fedf3461620b35 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 08:28:15 +0000 Subject: [PATCH] fix(app-shell): carry `default` and `visibleWhen` through the object-field options editor (objectui#7540) Editing any picklist option in the metadata-admin designer wrote the option back without its `default` or `visibleWhen` key. The payload stayed valid, so nothing surfaced -- silent data loss of two keys the platform honours, one of them (`default`) ruled `enforce` on the object-field face. The loss started in the READER. `readOptions` projected each authored option to exactly `value` / `label` / `color`, so both keys were gone before `patchOptions` ever saw them and no writer-only repair could have carried them. `readOptions` now keeps the whole authored option, parking the keys the editor has no control for on an internal carrier, and `patchOptions` spreads that carrier's contents back into the written document. This mirrors `readFields` one level up, which preserves unknown keys on a field definition and strips only the named retired keys a shipped build actually wrote. There is no counterpart tombstone list for option keys and none is owed: every entry in the registry is field-level, and this editor has only ever written the three keys it displays. No authoring UI changed -- per-option controls for these keys are a separate product question. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3 --- .changeset/7540-option-editor-key-loss.md | 18 + ...ldInspector.optionKeyPreservation.test.tsx | 329 ++++++++++++++++++ .../inspectors/ObjectFieldInspector.tsx | 93 ++++- 3 files changed, 433 insertions(+), 7 deletions(-) create mode 100644 .changeset/7540-option-editor-key-loss.md create mode 100644 packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.optionKeyPreservation.test.tsx diff --git a/.changeset/7540-option-editor-key-loss.md b/.changeset/7540-option-editor-key-loss.md new file mode 100644 index 0000000000..63d7849061 --- /dev/null +++ b/.changeset/7540-option-editor-key-loss.md @@ -0,0 +1,18 @@ +--- +'@object-ui/app-shell': patch +--- + +fix(app-shell): the object-field options editor no longer drops `default` and `visibleWhen` on save + +Opening a picklist field in the metadata-admin designer, editing any option and +saving used to write the option back without its `default` or `visibleWhen` +key. It was not a validation failure — the payload stayed perfectly valid, just +smaller than what the author wrote — so the loss was silent, and it took a +picklist default with it: `default` is `enforce` on the object-field face, and +the engine seeds the insert path from the option holding it. + +The loss started in the reader, not the writer. `readOptions` projected each +authored option down to `value` / `label` / `color`, so both keys were already +gone before `patchOptions` ran. `readOptions` now carries the keys the editor +has no control for and `patchOptions` writes them back, which also protects any +option key the spec accepts later. No authoring UI changed. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.optionKeyPreservation.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.optionKeyPreservation.test.tsx new file mode 100644 index 0000000000..772973e0c6 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.optionKeyPreservation.test.tsx @@ -0,0 +1,329 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Pins that a picklist option's `default` and `visibleWhen` SURVIVE a + * read-edit-write round trip through the field designer (objectui#7540). + * + * The bug was silent data loss, and it started in the READER: + * + * // readOptions -- the loss site + * return raw.map((o: any) => ({ + * value: String(o?.value ?? ''), + * label: typeof o?.label === 'string' ? o.label : undefined, + * color: typeof o?.color === 'string' ? o.color : undefined, + * })); + * + * Three keys in, three keys out — so `default` and `visibleWhen` were already + * gone by the time `patchOptions` ran, and no writer-only repair could have + * carried them. An author opened any picklist field, edited any option, saved, + * and both keys vanished from the document. Not a 422: the payload stayed + * perfectly valid, just smaller than what the author wrote. + * + * Re-measured here on `@objectstack/spec` 17.2.0, controls lit first: + * + * { value:'alpha', label:'A' } -> ACCEPT (clean control) + * { value:'a', label:'A' } -> REJECT too_small@[value] + * { value:'alpha', label:'A', default:true } -> ACCEPT + * { value:'alpha', label:'A', visibleWhen:'x > 1' } -> ACCEPT, canonicalized to + * {dialect:'cel',source:'x > 1'} + * { value:'alpha', label:'A', visibleWhen:true } -> REJECT invalid_union@[visibleWhen] + * { value:'alpha', label:'A', zzz:1 } -> REJECT unrecognized_keys@[] + * + * Two traps this file inherits rather than re-earning: + * • A select option's `value` must be at least 2 characters. A one-character + * value poisons every row with `too_small@[value]` and the rows actually + * about `visibleWhen` then say nothing — hence `alpha` / `beta`. + * • The BOOLEAN form of `visibleWhen` is refused; it is the STRING form that + * is accepted and canonicalized into the expression envelope. + * + * Every case is refusal-shaped: it asserts what the WRITTEN document carries + * and ends at `SelectOptionSchema` / `FieldSchema`, never at "it compiles". + * The suite also carries an emptiness control — a round trip that silently + * returned no options at all would pass a naive "the lost key is not present + * with a wrong value" assertion, so the option COUNT and the landed edit are + * asserted before any key is inspected. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, cleanup } 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(fields: Record, selectedId: string) { + const onPatch = vi.fn(); + render( + , + ); + /** The fields map the host's Save would persist after the last edit. */ + const savedFields = () => onPatch.mock.calls.at(-1)![0].fields as Record; + return { onPatch, savedFields }; +} + +/** The option list the designer would persist for `status`. */ +const savedOptions = (fields: Record): Def[] => + (fields.status as { options?: Def[] }).options ?? []; + +/** The whole field as the PUT body carries it (record key -> `name`). */ +const savedField = (fields: Record): Def => ({ name: 'status', ...fields.status }); + +/** 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[] } }; +}; + +/** Every issue the contract raises, as `code@[path]`, for readable failures. */ +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('.')}]`); +}; + +const selectField = (options: Def[]): Record => ({ + status: { type: 'select', label: 'Status', options }, +}); + +/** Edit the Label box of row `i` -- the round trip every case drives. */ +const editLabel = (i: number, to: string) => + fireEvent.change(screen.getAllByPlaceholderText('Label')[i], { target: { value: to } }); + +describe('ObjectFieldInspector · the option editor stops dropping `default` (objectui#7540)', () => { + it('`default: true` on an UNTOUCHED row survives an edit to another row', () => { + const { savedFields } = renderField( + selectField([ + { value: 'alpha', label: 'Alpha', default: true }, + { value: 'beta', label: 'Beta' }, + ]), + 'status', + ); + + editLabel(1, 'Beta II'); + + const options = savedOptions(savedFields()); + // EMPTINESS CONTROL, asserted before any key is inspected: a round trip + // that silently wrote no options at all would satisfy "the key does not + // hold a wrong value" vacuously. Both rows must still be there. + expect(options).toHaveLength(2); + // And the edit really landed -- so what follows measures a real round + // trip, not a render that never called the writer. + expect(options[1].label).toBe('Beta II'); + + expect(options[0].default).toBe(true); + expect(Object.prototype.hasOwnProperty.call(options[0], 'default')).toBe(true); + }); + + it('`default: true` survives on the very row being edited', () => { + const { savedFields } = renderField( + selectField([{ value: 'alpha', label: 'Alpha', default: true, color: '#ff0000' }]), + 'status', + ); + + editLabel(0, 'Alpha II'); + + const [option] = savedOptions(savedFields()); + expect(option).toEqual({ + value: 'alpha', + label: 'Alpha II', + color: '#ff0000', + default: true, + }); + }); + + it('the written option is ACCEPTED by SelectOptionSchema and FieldSchema', () => { + const { savedFields } = renderField( + selectField([ + { value: 'alpha', label: 'Alpha', default: true }, + { value: 'beta', label: 'Beta' }, + ]), + 'status', + ); + + editLabel(1, 'Beta II'); + + const fields = savedFields(); + for (const option of savedOptions(fields)) { + expect(rejectionsOf(SelectOptionSchema, option)).toEqual([]); + } + expect(rejectionsOf(FieldSchema, savedField(fields))).toEqual([]); + }); +}); + +describe('ObjectFieldInspector · the option editor stops dropping `visibleWhen` (objectui#7540)', () => { + it('the STRING form survives verbatim, and the contract canonicalizes it', () => { + const { savedFields } = renderField( + selectField([ + { value: 'alpha', label: 'Alpha', visibleWhen: 'record.tier == 2' }, + { value: 'beta', label: 'Beta' }, + ]), + 'status', + ); + + editLabel(1, 'Beta II'); + + const options = savedOptions(savedFields()); + expect(options).toHaveLength(2); + expect(options[1].label).toBe('Beta II'); + // Written back exactly as authored -- the editor carries, it does not + // rewrite. The canonicalization into the expression envelope is the + // CONTRACT's job, and the parsed result below shows it still happens. + expect(options[0].visibleWhen).toBe('record.tier == 2'); + + const parsed = SelectOptionSchema.safeParse(options[0]); + expect(parsed.success).toBe(true); + expect((parsed as { data: Def }).data.visibleWhen).toEqual({ + dialect: 'cel', + source: 'record.tier == 2', + }); + }); + + it('the ENVELOPE form survives verbatim', () => { + const { savedFields } = renderField( + selectField([ + { + value: 'alpha', + label: 'Alpha', + visibleWhen: { dialect: 'cel', source: 'record.tier == 2' }, + }, + { value: 'beta', label: 'Beta' }, + ]), + 'status', + ); + + editLabel(1, 'Beta II'); + + const options = savedOptions(savedFields()); + expect(options).toHaveLength(2); + expect(options[0].visibleWhen).toEqual({ dialect: 'cel', source: 'record.tier == 2' }); + expect(rejectionsOf(FieldSchema, savedField(savedFields()))).toEqual([]); + }); + + it('both keys ride together on one option, alongside the displayed ones', () => { + const { savedFields } = renderField( + selectField([ + { + value: 'alpha', + label: 'Alpha', + color: '#00ff00', + default: true, + visibleWhen: 'record.tier == 2', + }, + ]), + 'status', + ); + + editLabel(0, 'Alpha II'); + + const [option] = savedOptions(savedFields()); + expect(option).toEqual({ + value: 'alpha', + label: 'Alpha II', + color: '#00ff00', + default: true, + visibleWhen: 'record.tier == 2', + }); + expect(rejectionsOf(FieldSchema, savedField(savedFields()))).toEqual([]); + }); +}); + +describe('ObjectFieldInspector · the option round trip is measured, not assumed (objectui#7540)', () => { + it('the editor-internal carrier never reaches the document', () => { + // `readOptions` parks the keys it has no control for on an internal `rest` + // slot of its row type. That slot is NOT a document key: `SelectOptionSchema` + // is strict, so leaking it would 422 every save. Assert the exact key set, + // which a `hasOwnProperty('default')` check alone would not catch. + const { savedFields } = renderField( + selectField([{ value: 'alpha', label: 'Alpha', default: true }]), + 'status', + ); + + editLabel(0, 'Alpha II'); + + const [option] = savedOptions(savedFields()); + expect(Object.keys(option).sort()).toEqual(['default', 'label', 'value']); + expect(Object.prototype.hasOwnProperty.call(option, 'rest')).toBe(false); + }); + + it('the ACCEPTs above are not vacuous — the contract does refuse things', () => { + // Lit controls. Without these, every `toEqual([])` above could be green + // against a schema that waves everything through. + expect(rejectionsOf(SelectOptionSchema, { value: 'alpha', label: 'A' })).toEqual([]); + expect(rejectionsOf(SelectOptionSchema, { value: 'a', label: 'A' })).toEqual([ + 'too_small@[value]', + ]); + expect(rejectionsOf(SelectOptionSchema, { value: 'alpha', label: 'A', zzz: 1 })).toEqual([ + 'unrecognized_keys@[]', + ]); + // The boolean form is REFUSED; the string form is what canonicalizes. + expect(rejectionsOf(SelectOptionSchema, { value: 'alpha', label: 'A', visibleWhen: true })).toEqual([ + 'invalid_union@[visibleWhen]', + ]); + // And at the field level, where this editor's payload is actually judged. + expect( + rejectionsOf(FieldSchema, { + name: 'status', + type: 'select', + label: 'Status', + options: [{ value: 'alpha', label: 'A', zzz: 1 }], + }), + ).toEqual(['unrecognized_keys@[options.0]']); + }); + + it('an option with nothing extra is written exactly as before the repair', () => { + // Guards the other direction: carrying unknown keys must not have added a + // key, reordered the shape, or otherwise disturbed the plain case that + // objectui#7014 Q2 already pinned. + const { savedFields } = renderField(selectField([{ value: 'alpha', label: 'Alpha' }]), 'status'); + + editLabel(0, 'Alpha II'); + + expect(savedOptions(savedFields())).toEqual([{ value: 'alpha', label: 'Alpha II' }]); + }); + + it('a key the spec refuses is CARRIED, not silently laundered', () => { + // A deliberate consequence of preserving what the editor does not display, + // and the same contract `readFields` keeps one level up for field keys. + // + // The document below is ALREADY illegal before the inspector is opened -- + // asserted here, so this case cannot be read as the editor creating a 422. + // It declines to hide one. Silently dropping `zzz` would rewrite an + // author's document behind their back on a read they did not ask for, and + // there is no retired OPTION key for which that laundering is owed: every + // entry in the retired-key tombstone registry is FIELD-level, and this + // editor has only ever written `value` / `label` / `color`. + const authored = { value: 'alpha', label: 'Alpha', zzz: 1 }; + expect(rejectionsOf(SelectOptionSchema, authored)).toEqual(['unrecognized_keys@[]']); + + const { savedFields } = renderField(selectField([authored]), 'status'); + + editLabel(0, 'Alpha II'); + + const [option] = savedOptions(savedFields()); + expect(option.zzz).toBe(1); + }); +}); 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 8ac8dc220a..31c5cf3eea 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx @@ -15,7 +15,9 @@ * the object-fields-io helpers, preserving the original array-vs-record * shape AND any unknown keys on the field definition — except the * spec-rejected keys `object-fields-io` strips on read - * (`RETIRED_FIELD_KEYS`). + * (`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). * * There is deliberately no `Indexed` control here (objectui#4644): the * spec has no field-level index flag, `FieldSchema.safeParse` rejects @@ -65,22 +67,86 @@ import type { CelLintIssue } from '../celAuthoring.js'; import { t, tFormat } from '../i18n.js'; +/** + * One row of the picklist option editor. + * + * `value` / `label` / `color` are the keys this editor DISPLAYS and owns — it + * renders a control for each and is authoritative for their values. `rest` + * carries every OTHER authored key of the same option verbatim, so a round + * trip through this inspector is not lossy for keys it has no opinion about. + * Today that is `default` and `visibleWhen`: both are ACCEPTED by + * `SelectOptionSchema` and both are honoured by the platform (`default` is + * ruled `enforce` on the object-field face, objectstack#7246 — the engine + * seeds the insert path from the option holding it), yet neither had any way + * to survive this editor before objectui#7540. + * + * Two properties of `rest` are load-bearing: + * + * • It is EDITOR-INTERNAL and must never appear as a key of the WRITTEN + * option. `SelectOptionSchema` is strict (`unrecognized_keys` at + * `[options.]` on `FieldSchema`), so leaking it would turn every save + * into a 422. `patchOptions` therefore spreads its CONTENTS and never + * spreads an `Option`. + * • The three parts move TOGETHER. Widening this type without moving both + * `readOptions` and `patchOptions` would declare keys the editor still + * cannot carry — the declared-but-not-carried divergence objectui#7014 + * exists to remove. + */ interface Option { value: string; label?: string; color?: string; + /** Authored keys this editor has no control for, carried through untouched. */ + rest?: Record; } /* ─────────────── Helpers ─────────────── */ +/** + * Read `def.options` into editor rows, keeping the WHOLE authored option. + * + * The three displayed keys are normalized exactly as before; everything else + * on the authored option travels untouched in `rest`. + * + * This projection was the loss site (objectui#7540). It used to return exactly + * `value` / `label` / `color`, which is why a writer-only repair could not + * have closed the bug: whatever `patchOptions` were taught to carry, it can + * only carry what this function handed it, and this function handed it three + * keys. The reader is where `default` and `visibleWhen` disappeared. + * + * 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 + * actually wrote that the spec now refuses (`RETIRED_FIELD_KEYS`). There is no + * counterpart tombstone list for OPTION keys and none is owed: this editor has + * only ever written `value` / `label` / `color`, so no key it authored can + * come back as one the spec rejects. + */ function readOptions(def: Record): Option[] { const raw = def.options; if (!Array.isArray(raw)) return []; - return raw.map((o: any) => ({ - value: String(o?.value ?? ''), - label: typeof o?.label === 'string' ? o.label : undefined, - color: typeof o?.color === 'string' ? o.color : undefined, - })); + 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; + }); } function isPicklist(type: string): boolean { @@ -400,7 +466,20 @@ export function ObjectFieldInspector({ // 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. - const out: Option = { value: o.value, label: o.label ?? '' }; + // + // Everything the editor does NOT display rides along in `o.rest` + // (objectui#7540). It is spread FIRST so the three keys this editor owns + // are authoritative for their own slots; `readOptions` already removed + // them from `rest`, so that ordering is defence in depth rather than a + // live collision. Note the emitted option is a plain document, NOT an + // `Option` — `rest` is an editor-internal carrier and spreading the row + // itself would leak that key into the payload, which `SelectOptionSchema` + // rejects outright. + const out: Record = { + ...o.rest, + value: o.value, + label: o.label ?? '', + }; if (o.color) out.color = o.color; return out; });