From ac969aa156a94bb710fa62d536005f6992613da4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 10:50:14 +0000 Subject: [PATCH] refactor(app-shell): derive the action-location and directory-lookup vocabularies from the spec (#3017) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two hand-copied spec vocabularies in the metadata-admin inspectors, each under a comment asking the next author to keep it in step with `@objectstack/spec` and nothing enforcing it. Both agreed with the spec today; neither had a mechanism making that true tomorrow. objectui#3017 catalogues the pattern. **`ActionDefaultInspector.LOCATIONS`** restated all seven `ACTION_LOCATIONS` values. The labels stay here — they are presentation — but the vocabulary is now `Record`, which makes the compiler the mechanism: a location the spec ADDS is a missing-key error (TS2741) instead of a silently absent dropdown entry, and one it REMOVES is an excess-property error (TS2353). Both directions verified against the installed spec. Display order is the object's insertion order, so the authoring-friendly grouping is unchanged. **`FlowReferenceField.KIND_TO_RECORD_LOOKUP`** hard-coded `object` / `valueField` for the four directory-backed kinds. Its own comment said to "import it once a published release carries the export" — `@objectstack/spec@17` does, so it now composes objectui's presentation on top of `APPROVER_VALUE_SOURCES`. That table is where this pattern has already cost us once: the FIRST copy of the data contract wired every directory kind to the metadata registry, which holds no `sys_user` / `sys_team` / `sys_business_unit` / `sys_position` ROWS, so candidates came back empty, the picker degraded to free text, and `sales_manager` got typed into a field that accepts three values (framework#3508). Spec answered by publishing the binding behind a `satisfies` that makes an undeclared `ApproverType` a compile error. Reading it here is what carries that guarantee across the repo boundary. The split the spec asks for is preserved: it owns WHERE candidates come from and WHAT is committed; this package owns `displayField` / `picker` / `subtitle`. A kind whose source is not `data` is skipped rather than thrown on — a module-level throw would white-screen the designer over a vocabulary change. No behaviour change: the derived table is byte-identical to the literals it replaces. `FlowReferenceField.specDerivation.test.ts` pins that it stays wired (non-vacuous, object/valueField sourced from the spec, presentation preserved). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXGj3Z5TmwSV6RK2oGc3cb --- .../inspectors/ActionDefaultInspector.tsx | 37 +++++++--- .../FlowReferenceField.specDerivation.test.ts | 71 +++++++++++++++++++ .../inspectors/FlowReferenceField.tsx | 62 ++++++++++++++-- 3 files changed, 154 insertions(+), 16 deletions(-) create mode 100644 packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.specDerivation.test.ts diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx index 5eba73c4f7..ee9665ba36 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx @@ -30,6 +30,7 @@ import * as React from 'react'; import { Plus, Trash2 } from 'lucide-react'; +import type { ActionLocation } from '@objectstack/spec/ui'; import { Button, Label, Textarea, } from '@object-ui/components'; @@ -107,16 +108,32 @@ const PARAM_TYPE_OPTS = [ { value: 'lookup', label: 'Lookup' }, ]; -/** Canonical action locations (mirrors spec ACTION_LOCATIONS) with friendly labels. */ -const LOCATIONS: Array<{ value: string; label: string }> = [ - { value: 'record_header', label: 'Record header' }, - { value: 'record_more', label: 'Record · more menu' }, - { value: 'record_section', label: 'Record · section' }, - { value: 'record_related', label: 'Record · related list' }, - { value: 'list_toolbar', label: 'List toolbar' }, - { value: 'list_item', label: 'List · row' }, - { value: 'global_nav', label: 'Global nav / command palette' }, -]; +/** + * Friendly labels for the spec's action locations. + * + * The vocabulary belongs to `ActionLocation` (spec `ACTION_LOCATIONS`). This + * used to restate all seven values under a "mirrors spec ACTION_LOCATIONS" + * comment with nothing enforcing it (objectui#3017). Typing the map as a TOTAL + * `Record` makes the compiler the mechanism: a location + * the spec ADDS is a missing-key error here rather than a silently absent + * dropdown entry, and one it REMOVES is an excess-property error. + * + * Insertion order is the display order — an authoring-friendly grouping + * (record → list → global), deliberately not the spec's declaration order. + */ +const LOCATION_LABELS: Record = { + record_header: 'Record header', + record_more: 'Record · more menu', + record_section: 'Record · section', + record_related: 'Record · related list', + list_toolbar: 'List toolbar', + list_item: 'List · row', + global_nav: 'Global nav / command palette', +}; + +const LOCATIONS: Array<{ value: ActionLocation; label: string }> = ( + Object.keys(LOCATION_LABELS) as ActionLocation[] +).map((value) => ({ value, label: LOCATION_LABELS[value] })); /** Per-type binding hints for the single `target` field. */ const TARGET_FIELD: Record = { diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.specDerivation.test.ts b/packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.specDerivation.test.ts new file mode 100644 index 0000000000..daf6631f35 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.specDerivation.test.ts @@ -0,0 +1,71 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * **The directory-lookup table is derived from the spec, not restated** (objectui#3017). + * + * `KIND_TO_RECORD_LOOKUP` used to hard-code `object` / `valueField` for the four + * directory-backed kinds under a comment saying it mirrored the spec's + * `APPROVER_VALUE_BINDINGS`. It now composes objectui's presentation on top of + * the published `APPROVER_VALUE_SOURCES`, so those two fields cannot drift by + * construction. + * + * What is still worth asserting is that the composition stays WIRED. Two ways it + * could quietly stop being: the derivation could yield an empty table (every + * kind skipped because the spec changed shape), or a kind could lose its + * `data` source and vanish from the picker. Both would degrade a resolvable + * reference back to a free-text box — exactly the framework #3508 failure that + * publishing the binding existed to end, and exactly the kind of regression a + * comment cannot catch. + */ + +import { describe, it, expect } from 'vitest'; +import { APPROVER_VALUE_SOURCES } from '@objectstack/spec/automation'; + +import { KIND_TO_RECORD_LOOKUP } from './FlowReferenceField'; + +/** The directory-backed kinds this package renders a record lookup for. */ +const EXPECTED_KINDS = ['user', 'team', 'department', 'position'] as const; + +describe('KIND_TO_RECORD_LOOKUP is derived from APPROVER_VALUE_SOURCES (objectui#3017)', () => { + it('is not vacuous — the derivation still produces every expected kind', () => { + // If the spec renamed these keys or changed their `source`, the flatMap + // would skip them and the table would silently shrink. + expect(Object.keys(KIND_TO_RECORD_LOOKUP).sort()).toEqual([...EXPECTED_KINDS].sort()); + }); + + it.each(EXPECTED_KINDS)('%s: object + valueField come from the spec, not a local literal', (kind) => { + const source = APPROVER_VALUE_SOURCES[kind as keyof typeof APPROVER_VALUE_SOURCES]; + expect(source, `spec no longer declares an approver binding for '${kind}'`).toBeDefined(); + expect( + source.source, + `spec changed '${kind}' away from a record lookup — the picker needs rethinking, not just re-deriving`, + ).toBe('data'); + + const binding = KIND_TO_RECORD_LOOKUP[kind]; + expect(binding).toBeDefined(); + expect(binding!.object).toBe((source as { object: string }).object); + expect(binding!.valueField).toBe((source as { valueField: string }).valueField); + }); + + it('keeps presentation local — the spec publishes no display hints', () => { + // The split is the point: the spec owns WHERE candidates come from and WHAT + // is committed; this package owns what the row looks like. A spec that + // started publishing display hints would make this assertion fail and force + // the boundary to be re-drawn deliberately. + for (const kind of EXPECTED_KINDS) { + const source = APPROVER_VALUE_SOURCES[kind as keyof typeof APPROVER_VALUE_SOURCES] as Record; + expect(Object.keys(source).sort()).toEqual(['object', 'source', 'valueField']); + expect(KIND_TO_RECORD_LOOKUP[kind]!.displayField).toBeTruthy(); + } + }); + + it('the people picker stays on `user` only', () => { + // `picker: 'search'` opts into avatar rows; it is presentation, so it must + // survive the derivation rather than be lost when the binding is composed. + expect(KIND_TO_RECORD_LOOKUP.user?.picker).toBe('search'); + expect(KIND_TO_RECORD_LOOKUP.user?.subtitle).toEqual(['email']); + for (const kind of ['team', 'department', 'position'] as const) { + expect(KIND_TO_RECORD_LOOKUP[kind]?.picker).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 c182f8f1a4..f63a6f9f16 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.tsx @@ -39,6 +39,7 @@ import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue, } from '@object-ui/components'; import { Pencil, Search } from 'lucide-react'; +import { APPROVER_VALUE_SOURCES } from '@objectstack/spec/automation'; import { useAdapter } from '@object-ui/react'; import { LookupField } from '@object-ui/fields'; import type { FlowReferenceSpec, ReferenceKind, RefValueSource } from './flow-node-config'; @@ -100,12 +101,61 @@ export interface RecordLookupBinding { /** Secondary-line fields for people rows. */ subtitle?: string[]; } -export const KIND_TO_RECORD_LOOKUP: Partial> = { - user: { object: 'sys_user', valueField: 'id', displayField: 'name', picker: 'search', subtitle: ['email'] }, - team: { object: 'sys_team', valueField: 'id', displayField: 'name' }, - department: { object: 'sys_business_unit', valueField: 'id', displayField: 'name' }, - position: { object: 'sys_position', valueField: 'name', displayField: 'label' }, -}; +/** + * PRESENTATION for the directory-backed kinds — which column to show, whether + * to open the people picker, what subtitle to put under a row. + * + * Deliberately local: the spec publishes the data contract only and says so. + * Everything below this line is objectui's call; everything above it (which + * object, which column is committed) now comes from the spec. + */ +const RECORD_LOOKUP_PRESENTATION = { + user: { displayField: 'name', picker: 'search', subtitle: ['email'] }, + team: { displayField: 'name' }, + department: { displayField: 'name' }, + position: { displayField: 'label' }, +} as const satisfies Partial< + Record> +>; + +/** + * The older-server fallback, DERIVED from the spec's published binding. + * + * `object` / `valueField` used to be hand-written here, under a comment saying + * this table "mirrors `APPROVER_VALUE_BINDINGS` … import it once a published + * release carries the export". `@objectstack/spec@17` carries it, so this now + * reads `APPROVER_VALUE_SOURCES` and composes objectui's presentation on top + * (objectui#3017). + * + * That matters more here than anywhere else in the designer: the FIRST copy of + * this data contract was wrong — every directory kind was wired to the metadata + * registry, which holds no `sys_user` / `sys_team` / `sys_business_unit` / + * `sys_position` ROWS, so candidates came back empty, the control degraded to + * free text, and `sales_manager` got typed into a field that accepts three + * values (framework #3508). Spec answered by publishing the binding with a + * `satisfies` that makes an undeclared `ApproverType` a compile error; reading + * it here is what finally carries that guarantee across the repo boundary. + * + * A kind whose spec source is not `data` is skipped rather than thrown on — a + * module-level throw would white-screen the designer over a vocabulary change. + * `FlowReferenceField.specDerivation.test.ts` asserts the table is non-vacuous + * and still covers every kind with presentation. + */ +export const KIND_TO_RECORD_LOOKUP: Partial> = + Object.fromEntries( + Object.entries(RECORD_LOOKUP_PRESENTATION).flatMap(([kind, presentation]) => { + const source = APPROVER_VALUE_SOURCES[kind as keyof typeof APPROVER_VALUE_SOURCES]; + if (!source || source.source !== 'data') return []; + return [[ + kind, + { + object: source.object, + valueField: source.valueField as RecordLookupBinding['valueField'], + ...presentation, + }, + ]]; + }), + ); /** * better-auth org-membership tiers. `org-membership-level` is intentionally NOT