diff --git a/.changeset/approver-value-sources-from-schema.md b/.changeset/approver-value-sources-from-schema.md new file mode 100644 index 000000000..683a9d61b --- /dev/null +++ b/.changeset/approver-value-sources-from-schema.md @@ -0,0 +1,36 @@ +--- +"@object-ui/app-shell": patch +--- + +fix(flow-designer): read approver value sources off the schema instead of mirroring them (framework#3508 follow-up) + +The approver Value picker decided *where its candidates live* from a local +table, `KIND_TO_RECORD_LOOKUP`, hand-mirrored from the spec's +`APPROVER_VALUE_BINDINGS`. That mirror is what made framework#3508 possible: +`xRef.map` names a picker KIND (`'team'`) and nothing more, so this package had +to pick a data source itself — and picked the metadata REGISTRY +(`GET /api/v1/meta/:type`), which lists no `sys_user` / `sys_team` / +`sys_business_unit` / `sys_position` ROWS. Candidates were always empty and the +control degraded to a raw-id text box. + +The spec now publishes the data contract as `xRef.sources` (one entry per +approver type: `{ source: 'data', object, valueField }`, the closed enum +inline, or a non-picker marker). `json-schema-to-fields` carries it through — +validating each entry, dropping any that could not drive a picker — and +`recordLookupFor()` prefers it over the local table. A new approver type can no +longer leave a stale mirror behind here. + +What did NOT move: presentation. Which field to display, whether to open the +people picker, what subtitle to show under a row stay this package's calls, so +the spec ships the data contract and not the look. The local table remains as +the fallback for a server that predates the annotation, and a `data` source for +a kind with no presentation entry still renders a lookup labelled by its +committed column — better than degrading a resolvable reference to free text. + +Also corrects the approver `type` options comment in `flow-node-config.ts`: that +list is the OFFLINE fallback (`FlowNodeInspector` renders +`serverFields ?? fieldsForNodeType(...)`, so a real backend's published +configSchema wins). Its "indirect bindings lead, `user` last" ordering therefore +never reached the live picker, which followed the spec enum with `user` first — +the opposite of the intent. The ordering now lives in the spec's `ApproverType` +enum, and the comment says which list is authoritative. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.sources.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.sources.test.tsx new file mode 100644 index 000000000..5f47664a2 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.sources.test.tsx @@ -0,0 +1,105 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * framework #3508 follow-up — the approver Value picker now reads WHERE its + * candidates live off the published schema (`xRef.sources`) instead of a local + * mirror of the spec's `APPROVER_VALUE_BINDINGS`. + * + * The mirror is what made #3508 possible in the first place: `xRef.map` named + * a picker KIND and nothing more, so this package had to decide the data + * source itself — and decided wrong, wiring every directory kind to the + * metadata registry, which holds no `sys_user` / `sys_team` rows. + * + * Load-bearing behaviours: + * 1. a published `data` source WINS over the local table (that is what stops + * the mirror from drifting from the engine again); + * 2. no published source → the local table still drives the picker, so an + * older server keeps working; + * 3. a published NON-data source (enum / auto / unsupported) keeps the kind + * off the record-lookup path entirely; + * 4. presentation (display field, people-picker, subtitle) stays local — the + * spec ships the data contract, not the look; + * 5. the source is keyed by the DISCRIMINATOR, not the picker kind, so + * `role` and `org_membership_level` stay distinguishable though they + * share one kind. + */ + +import { describe, it, expect } from 'vitest'; +import { recordLookupFor, resolveRefKind, KIND_TO_RECORD_LOOKUP } from './FlowReferenceField'; + +describe('recordLookupFor — published source vs local mirror (#3508 follow-up)', () => { + it('prefers the schema’s object and committed column over the local table', () => { + // A server that renamed the directory object: the picker must follow the + // SCHEMA, or it queries a table the engine does not resolve against. + const lookup = recordLookupFor({ + kind: 'user', + source: { source: 'data', object: 'sys_person', valueField: 'external_id' }, + }); + expect(lookup).toMatchObject({ object: 'sys_person', valueField: 'external_id' }); + // …while presentation still comes from here. + expect(lookup?.picker).toBe('search'); + expect(lookup?.subtitle).toEqual(['email']); + }); + + it('falls back to the local table when the server publishes no source', () => { + expect(recordLookupFor({ kind: 'department' })).toEqual(KIND_TO_RECORD_LOOKUP.department); + expect(recordLookupFor({ kind: 'position' })).toEqual(KIND_TO_RECORD_LOOKUP.position); + }); + + it('keeps a published non-data source off the record path', () => { + // These are not pickers over rows: a closed enum, a runtime-resolved + // value, and a type the runtime never resolves at all. + expect(recordLookupFor({ kind: 'org-membership-level', source: { source: 'enum', values: ['owner'] } })).toBeUndefined(); + expect(recordLookupFor({ kind: 'manager', source: { source: 'auto' } })).toBeUndefined(); + expect(recordLookupFor({ kind: 'queue', source: { source: 'unsupported' } })).toBeUndefined(); + }); + + it('renders a lookup for a data source this package has no presentation entry for', () => { + // Better a working picker labelled by its committed column than degrading + // a perfectly resolvable reference to free text. + const lookup = recordLookupFor({ + kind: 'queue', + source: { source: 'data', object: 'sys_queue', valueField: 'id' }, + }); + expect(lookup).toEqual({ object: 'sys_queue', valueField: 'id', displayField: 'id' }); + }); + + it('resolves nothing for an unresolved reference', () => { + expect(recordLookupFor(undefined)).toBeUndefined(); + }); +}); + +describe('resolveRefKind — sources are keyed by the discriminator (#3508 follow-up)', () => { + const ref = { + kindFrom: 'type', + map: { user: 'user', role: 'org-membership-level', org_membership_level: 'org-membership-level' }, + sources: { + user: { source: 'data' as const, object: 'sys_user', valueField: 'id' }, + role: { source: 'enum' as const, values: ['owner', 'admin', 'member'] }, + org_membership_level: { source: 'enum' as const, values: ['owner', 'admin', 'member'] }, + }, + }; + + it('carries the source for the row’s own type', () => { + const resolved = resolveRefKind(ref, () => 'user'); + expect(resolved).toMatchObject({ kind: 'user', source: { source: 'data', object: 'sys_user' } }); + }); + + it('distinguishes two discriminators that share one picker kind', () => { + // `role` is the deprecated spelling of `org_membership_level`; both render + // the same control, and a per-KIND lookup could not tell them apart. + expect(resolveRefKind(ref, () => 'role')?.source).toEqual({ source: 'enum', values: ['owner', 'admin', 'member'] }); + expect(resolveRefKind(ref, () => 'org_membership_level')?.kind).toBe('org-membership-level'); + }); + + it('leaves the source undefined when the server published none', () => { + const bare = { kindFrom: 'type', map: { user: 'user' as const } }; + expect(resolveRefKind(bare, () => 'user')).toEqual({ + kind: 'user', objectSource: undefined, connectorSource: undefined, source: undefined, + }); + }); + + it('still returns undefined for a discriminator with no mapped kind', () => { + expect(resolveRefKind(ref, () => 'nope')).toBeUndefined(); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.tsx index 47d7743a8..c182f8f1a 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.tsx @@ -41,7 +41,7 @@ import { import { Pencil, Search } from 'lucide-react'; import { useAdapter } from '@object-ui/react'; import { LookupField } from '@object-ui/fields'; -import type { FlowReferenceSpec, ReferenceKind } from './flow-node-config'; +import type { FlowReferenceSpec, ReferenceKind, RefValueSource } from './flow-node-config'; import { useMetadataClient } from '../useMetadata'; import { useObjectFields } from '../previews/useObjectFields'; @@ -127,6 +127,13 @@ export interface ResolvedRef { kind: ReferenceKind; objectSource?: string; connectorSource?: string; + /** + * The schema's own answer for where this kind's candidates live, when the + * server published one (framework#3508 follow-up). Takes precedence over + * {@link KIND_TO_RECORD_LOOKUP}, which stays as the fallback for a server + * that predates the annotation. + */ + source?: RefValueSource; } /** @@ -140,15 +147,61 @@ export function resolveRefKind( sibling: (key: string) => unknown, ): ResolvedRef | undefined { if (!ref) return undefined; - if (ref.kind) return { kind: ref.kind, objectSource: ref.objectSource, connectorSource: ref.connectorSource }; + if (ref.kind) { + return { + kind: ref.kind, + objectSource: ref.objectSource, + connectorSource: ref.connectorSource, + source: ref.sources?.[ref.kind], + }; + } if (ref.kindFrom && ref.map) { const disc = sibling(ref.kindFrom); const k = typeof disc === 'string' ? ref.map[disc] : undefined; - if (k) return { kind: k, objectSource: ref.objectSource, connectorSource: ref.connectorSource }; + // The source is keyed by the DISCRIMINATOR (the approver `type`), not by + // the picker kind: `role` and `org_membership_level` share one kind but + // are separate enum members upstream. + if (k) { + return { + kind: k, + objectSource: ref.objectSource, + connectorSource: ref.connectorSource, + source: typeof disc === 'string' ? ref.sources?.[disc] : undefined, + }; + } } return undefined; } +/** + * The record lookup to render for a resolved reference. + * + * The schema's `xRef.sources` wins when present — it is the spec's own + * `APPROVER_VALUE_BINDINGS`, so it cannot drift from what the engine resolves. + * {@link KIND_TO_RECORD_LOOKUP} supplies the fallback (older server) and, in + * both paths, the PRESENTATION: which field to show, whether to open the + * people picker, what subtitle to put under a row. Those are this package's + * calls and deliberately stay out of the spec. + * + * A `data` source for a kind this package has no presentation entry for still + * renders a lookup — display falls back to the committed column, which beats + * degrading a resolvable reference to free text. + */ +export function recordLookupFor(resolved: ResolvedRef | undefined): RecordLookupBinding | undefined { + if (!resolved) return undefined; + const local = KIND_TO_RECORD_LOOKUP[resolved.kind]; + const published = resolved.source; + if (published && published.source !== 'data') return undefined; + if (!published) return local; + return { + object: published.object, + valueField: published.valueField as RecordLookupBinding['valueField'], + displayField: local?.displayField ?? published.valueField, + ...(local?.picker ? { picker: local.picker } : {}), + ...(local?.subtitle ? { subtitle: local.subtitle } : {}), + }; +} + /** Read `node.config[key]` as a non-empty string, else undefined. */ function configString(node: Record | null | undefined, key: string): string | undefined { const cfg = node?.config; @@ -518,7 +571,10 @@ export function ReferenceCombobox({ resolved, value, onCommit, onBlur, onSelect, // (All hooks above have already run — the branches below only render.) // Directory-backed kinds → single-select record lookup (framework #3508). - const lookup = kind ? KIND_TO_RECORD_LOOKUP[kind] : undefined; + // The object + committed column come from the schema when the server + // publishes them, so this package no longer decides where the engine's + // approvers live — see `recordLookupFor`. + const lookup = recordLookupFor(resolved); if (lookup) { return ( ; + /** + * Where each discriminator value's candidates actually live, keyed like + * {@link map} — published by the spec as `xRef.sources` (framework #3508 + * follow-up) and carried through by `json-schema-to-fields`. + * + * {@link map} only ever named a picker KIND. It never said what backs that + * picker, so this package had to keep its own copy of the data contract — + * and the first copy pointed every directory kind at the metadata REGISTRY + * (`GET /api/v1/meta/:type`), which cannot list `sys_user` / `sys_team` / + * `sys_business_unit` / `sys_position` ROWS. Candidates came back empty and + * the control silently degraded to free text (framework#3508). Reading the + * source off the schema means a new approver type can no longer leave a + * stale mirror behind here. + * + * Absent when the server predates the annotation — consumers keep a local + * fallback for that case. + */ + sources?: Record; } +/** + * How a polymorphic reference's candidates are sourced, per discriminator + * value. Mirrors the spec's `APPROVER_VALUE_SOURCES` projection. + * + * `data` means the DATA API (`/api/v1/data/:object`) — named in deliberate + * contrast to `meta`, the registry this used to query by mistake. The other + * variants are not pickers at all: a closed `enum`, an `auto`-resolved value, a + * `trigger-field` name, a CEL `expression`, or an `unsupported` type the + * runtime never resolves. + */ +export type RefValueSource = + | { source: 'data'; object: string; valueField: string } + | { source: 'enum'; values: string[] } + | { source: 'auto' | 'trigger-field' | 'expression' | 'unsupported' }; + /** Column descriptor for an `objectList` repeater row. */ export interface FlowConfigColumn { key: string; @@ -537,14 +570,25 @@ const FLOW_NODE_CONFIG: Record = { key: 'type', label: 'Type', kind: 'select', - // Mirrors the spec's NON_AUTHORABLE_APPROVER_TYPES (approval.zod.ts): - // `role` is the deprecated spelling of `org_membership_level` - // (ADR-0090 D3), and `queue` is declared-but-unenforced — the runtime - // resolves it to nobody (framework #3508). A stored row of either - // still renders (the select surfaces it flagged "(deprecated)"), but - // the designer never authors a new one. Indirect bindings lead and - // the literal `user` binding comes last: binding a specific person is - // the least portable choice (env moves, people leave). + // OFFLINE FALLBACK ONLY. `FlowNodeInspector` renders + // `serverFields ?? fieldsForNodeType(...)`, so against a real backend + // the approval node's fields come from the engine-published + // configSchema and this list is never read — it covers the preview + // gallery and any stack whose server publishes no schema. + // + // That is why the ordering below is ALSO carried by the spec's + // `ApproverType` enum (framework#3508 follow-up): stating it only + // here left the live picker in enum order with `user` first, the + // exact opposite of the intent. Indirect bindings lead and the + // literal `user` binding comes last — binding a specific person is + // the least portable choice (env moves, people leave). Keep the two + // in sync; the spec is the source of truth. + // + // Both paths also drop the spec's NON_AUTHORABLE_APPROVER_TYPES — + // `role` (deprecated spelling of `org_membership_level`, ADR-0090 D3) + // and `queue` (declared-but-unenforced; the runtime resolves it to + // nobody, framework#3508). A stored row of either still renders, + // flagged "(deprecated)", but neither is offered for new authoring. options: [ { value: 'manager', label: 'Manager' }, { value: 'position', label: 'Position' }, diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.test.ts b/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.test.ts index 4ea1775e7..c19904afa 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.test.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.test.ts @@ -155,6 +155,67 @@ describe('jsonSchemaToFlowFields', () => { expect(col.ref).toEqual({ kind: 'connector' }); }); + // framework#3508 follow-up: `map` names a picker KIND but never said what + // backs it, so this package had to guess the data source — and guessed the + // metadata registry, which lists no directory ROWS. `sources` carries the + // answer, and an entry the designer cannot act on is dropped rather than + // half-trusted (a `data` source with no committed column would query + // nothing, which is the failure the annotation exists to end). + it('carries xRef.sources through, dropping entries that could not drive a picker', () => { + const fields = jsonSchemaToFlowFields({ + type: 'object', + properties: { + rows: { + type: 'array', + items: { + type: 'object', + properties: { + value: { + type: 'string', + xRef: { + kindFrom: 'type', + map: { user: 'user', team: 'team', manager: 'manager' }, + sources: { + user: { source: 'data', object: 'sys_user', valueField: 'id' }, + manager: { source: 'auto' }, + org_membership_level: { source: 'enum', values: ['owner', 'admin'] }, + // Unusable: a record source with no committed column. + team: { source: 'data', object: 'sys_team' }, + // Unusable: not a source shape at all. + bogus: { source: 'telepathy' }, + }, + }, + }, + }, + }, + }, + }, + })!; + const col = fields.find((f) => f.id === 'rows')!.columns!.find((c) => c.key === 'value')!; + expect(col.ref!.sources).toEqual({ + user: { source: 'data', object: 'sys_user', valueField: 'id' }, + manager: { source: 'auto' }, + org_membership_level: { source: 'enum', values: ['owner', 'admin'] }, + }); + }); + + it('omits sources entirely for a server that predates the annotation', () => { + const fields = jsonSchemaToFlowFields({ + type: 'object', + properties: { + rows: { + type: 'array', + items: { + type: 'object', + properties: { value: { type: 'string', xRef: { kindFrom: 'type', map: { user: 'user' } } } }, + }, + }, + }, + })!; + const col = fields.find((f) => f.id === 'rows')!.columns!.find((c) => c.key === 'value')!; + expect(col.ref).toEqual({ kindFrom: 'type', map: { user: 'user' } }); + }); + it('drops a polymorphic xRef whose map has no known kinds (column stays text)', () => { const fields = jsonSchemaToFlowFields({ type: 'object', diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.ts b/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.ts index a25a0494e..50f7e32a0 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.ts @@ -40,7 +40,7 @@ * e.g. CRUD `fields`/`filter`, assignments) */ -import type { FlowConfigField, FlowConfigColumn, FlowConfigFieldKind, FlowReferenceSpec, ReferenceKind } from './flow-node-config'; +import type { FlowConfigField, FlowConfigColumn, FlowConfigFieldKind, FlowReferenceSpec, ReferenceKind, RefValueSource } from './flow-node-config'; /** Loose JSON Schema node shape — we only read the keys we map. */ interface JsonSchemaNode { @@ -78,7 +78,20 @@ interface JsonSchemaNode { * (`kindFrom` + `map`): the concrete kind is chosen at render time from a * sibling field/column value (e.g. an approver's `value` follows its `type`). */ - xRef?: { kind?: string; objectSource?: string; kindFrom?: string; map?: Record }; + xRef?: { + kind?: string; + objectSource?: string; + kindFrom?: string; + map?: Record; + /** + * Per-discriminator data contract (framework #3508 follow-up): where each + * kind's candidates live and which column the picker commits. `map` names + * only a picker KIND, which is how this package came to query the metadata + * REGISTRY for data records; with `sources` the designer reads the answer + * off the schema instead of mirroring it locally. + */ + sources?: Record; + }; /** * Authoring-semantics marker carried from the executor's Zod * `.meta({ xExpression })` (ADR-0018), on a string property. Declares whether @@ -110,6 +123,42 @@ const REFERENCE_KINDS: ReadonlySet = new Set([ 'email-template', ]); +/** + * Validate one `xRef.sources` entry. An entry the designer cannot act on is + * DROPPED rather than half-trusted: a `data` source missing its object or + * committed column would produce a picker that queries nothing, which is the + * failure this annotation exists to end (framework#3508). A dropped entry + * simply leaves the consumer on its local fallback. + */ +function sourceOf(raw: unknown): RefValueSource | undefined { + if (!raw || typeof raw !== 'object') return undefined; + const s = raw as Record; + if (s.source === 'data') { + return typeof s.object === 'string' && s.object && typeof s.valueField === 'string' && s.valueField + ? { source: 'data', object: s.object, valueField: s.valueField } + : undefined; + } + if (s.source === 'enum') { + const values = Array.isArray(s.values) ? s.values.filter((v): v is string => typeof v === 'string') : []; + return values.length ? { source: 'enum', values } : undefined; + } + if (s.source === 'auto' || s.source === 'trigger-field' || s.source === 'expression' || s.source === 'unsupported') { + return { source: s.source }; + } + return undefined; +} + +/** Read the per-discriminator data contract, or undefined when none survives. */ +function sourcesOf(raw: unknown): Record | undefined { + if (!raw || typeof raw !== 'object') return undefined; + const out: Record = {}; + for (const [disc, entry] of Object.entries(raw as Record)) { + const parsed = sourceOf(entry); + if (parsed) out[disc] = parsed; + } + return Object.keys(out).length ? out : undefined; +} + /** * Read a valid `xRef` annotation off a schema node, or undefined. Accepts both * the static shape (`{ kind }`) and the polymorphic shape (`{ kindFrom, map }`), @@ -128,7 +177,10 @@ function refOf(node: JsonSchemaNode): FlowReferenceSpec | undefined { if (typeof kind === 'string' && REFERENCE_KINDS.has(kind)) map[disc] = kind as ReferenceKind; } if (Object.keys(map).length === 0) return undefined; - return { kindFrom: x.kindFrom, map, ...objectSource }; + // `sources` is optional and additive: a server that predates it simply + // leaves the consumer on its local fallback (framework#3508 follow-up). + const sources = sourcesOf(x.sources); + return { kindFrom: x.kindFrom, map, ...objectSource, ...(sources ? { sources } : {}) }; } // Static: a single concrete kind.