Skip to content

Commit bf66b70

Browse files
authored
refactor(app-shell): derive the action-location and directory-lookup vocabularies from the spec (#3017) (#3019)
Two hand-copied spec vocabularies in the metadata-admin inspectors, each under a "keep in sync" comment with nothing enforcing it. Both agreed with the spec; neither had a mechanism making that true tomorrow. ActionDefaultInspector.LOCATIONS restated all seven ACTION_LOCATIONS values. The labels stay local (presentation), but the vocabulary is now a total Record<ActionLocation, string>, so a location the spec adds is a missing-key error and one it removes is an excess-property error. FlowReferenceField.KIND_TO_RECORD_LOOKUP hard-coded object/valueField for the four directory-backed kinds. Its own comment said to import once a published release carried the export; @objectstack/spec@17 does, so it now composes objectui's presentation on top of APPROVER_VALUE_SOURCES. That table is where the pattern already cost us once (framework#3508). The spec's split is preserved: it owns where candidates come from and what is committed; this package owns displayField / picker / subtitle. No behaviour change — the derived table is byte-identical to the literals it replaces. FlowReferenceField.specDerivation.test.ts pins that it stays wired.
1 parent 2baa13f commit bf66b70

3 files changed

Lines changed: 154 additions & 16 deletions

File tree

packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030

3131
import * as React from 'react';
3232
import { Plus, Trash2 } from 'lucide-react';
33+
import type { ActionLocation } from '@objectstack/spec/ui';
3334
import {
3435
Button, Label, Textarea,
3536
} from '@object-ui/components';
@@ -107,16 +108,32 @@ const PARAM_TYPE_OPTS = [
107108
{ value: 'lookup', label: 'Lookup' },
108109
];
109110

110-
/** Canonical action locations (mirrors spec ACTION_LOCATIONS) with friendly labels. */
111-
const LOCATIONS: Array<{ value: string; label: string }> = [
112-
{ value: 'record_header', label: 'Record header' },
113-
{ value: 'record_more', label: 'Record · more menu' },
114-
{ value: 'record_section', label: 'Record · section' },
115-
{ value: 'record_related', label: 'Record · related list' },
116-
{ value: 'list_toolbar', label: 'List toolbar' },
117-
{ value: 'list_item', label: 'List · row' },
118-
{ value: 'global_nav', label: 'Global nav / command palette' },
119-
];
111+
/**
112+
* Friendly labels for the spec's action locations.
113+
*
114+
* The vocabulary belongs to `ActionLocation` (spec `ACTION_LOCATIONS`). This
115+
* used to restate all seven values under a "mirrors spec ACTION_LOCATIONS"
116+
* comment with nothing enforcing it (objectui#3017). Typing the map as a TOTAL
117+
* `Record<ActionLocation, string>` makes the compiler the mechanism: a location
118+
* the spec ADDS is a missing-key error here rather than a silently absent
119+
* dropdown entry, and one it REMOVES is an excess-property error.
120+
*
121+
* Insertion order is the display order — an authoring-friendly grouping
122+
* (record → list → global), deliberately not the spec's declaration order.
123+
*/
124+
const LOCATION_LABELS: Record<ActionLocation, string> = {
125+
record_header: 'Record header',
126+
record_more: 'Record · more menu',
127+
record_section: 'Record · section',
128+
record_related: 'Record · related list',
129+
list_toolbar: 'List toolbar',
130+
list_item: 'List · row',
131+
global_nav: 'Global nav / command palette',
132+
};
133+
134+
const LOCATIONS: Array<{ value: ActionLocation; label: string }> = (
135+
Object.keys(LOCATION_LABELS) as ActionLocation[]
136+
).map((value) => ({ value, label: LOCATION_LABELS[value] }));
120137

121138
/** Per-type binding hints for the single `target` field. */
122139
const TARGET_FIELD: Record<string, { label: string; placeholder: string; hint: string }> = {
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* **The directory-lookup table is derived from the spec, not restated** (objectui#3017).
5+
*
6+
* `KIND_TO_RECORD_LOOKUP` used to hard-code `object` / `valueField` for the four
7+
* directory-backed kinds under a comment saying it mirrored the spec's
8+
* `APPROVER_VALUE_BINDINGS`. It now composes objectui's presentation on top of
9+
* the published `APPROVER_VALUE_SOURCES`, so those two fields cannot drift by
10+
* construction.
11+
*
12+
* What is still worth asserting is that the composition stays WIRED. Two ways it
13+
* could quietly stop being: the derivation could yield an empty table (every
14+
* kind skipped because the spec changed shape), or a kind could lose its
15+
* `data` source and vanish from the picker. Both would degrade a resolvable
16+
* reference back to a free-text box — exactly the framework #3508 failure that
17+
* publishing the binding existed to end, and exactly the kind of regression a
18+
* comment cannot catch.
19+
*/
20+
21+
import { describe, it, expect } from 'vitest';
22+
import { APPROVER_VALUE_SOURCES } from '@objectstack/spec/automation';
23+
24+
import { KIND_TO_RECORD_LOOKUP } from './FlowReferenceField';
25+
26+
/** The directory-backed kinds this package renders a record lookup for. */
27+
const EXPECTED_KINDS = ['user', 'team', 'department', 'position'] as const;
28+
29+
describe('KIND_TO_RECORD_LOOKUP is derived from APPROVER_VALUE_SOURCES (objectui#3017)', () => {
30+
it('is not vacuous — the derivation still produces every expected kind', () => {
31+
// If the spec renamed these keys or changed their `source`, the flatMap
32+
// would skip them and the table would silently shrink.
33+
expect(Object.keys(KIND_TO_RECORD_LOOKUP).sort()).toEqual([...EXPECTED_KINDS].sort());
34+
});
35+
36+
it.each(EXPECTED_KINDS)('%s: object + valueField come from the spec, not a local literal', (kind) => {
37+
const source = APPROVER_VALUE_SOURCES[kind as keyof typeof APPROVER_VALUE_SOURCES];
38+
expect(source, `spec no longer declares an approver binding for '${kind}'`).toBeDefined();
39+
expect(
40+
source.source,
41+
`spec changed '${kind}' away from a record lookup — the picker needs rethinking, not just re-deriving`,
42+
).toBe('data');
43+
44+
const binding = KIND_TO_RECORD_LOOKUP[kind];
45+
expect(binding).toBeDefined();
46+
expect(binding!.object).toBe((source as { object: string }).object);
47+
expect(binding!.valueField).toBe((source as { valueField: string }).valueField);
48+
});
49+
50+
it('keeps presentation local — the spec publishes no display hints', () => {
51+
// The split is the point: the spec owns WHERE candidates come from and WHAT
52+
// is committed; this package owns what the row looks like. A spec that
53+
// started publishing display hints would make this assertion fail and force
54+
// the boundary to be re-drawn deliberately.
55+
for (const kind of EXPECTED_KINDS) {
56+
const source = APPROVER_VALUE_SOURCES[kind as keyof typeof APPROVER_VALUE_SOURCES] as Record<string, unknown>;
57+
expect(Object.keys(source).sort()).toEqual(['object', 'source', 'valueField']);
58+
expect(KIND_TO_RECORD_LOOKUP[kind]!.displayField).toBeTruthy();
59+
}
60+
});
61+
62+
it('the people picker stays on `user` only', () => {
63+
// `picker: 'search'` opts into avatar rows; it is presentation, so it must
64+
// survive the derivation rather than be lost when the binding is composed.
65+
expect(KIND_TO_RECORD_LOOKUP.user?.picker).toBe('search');
66+
expect(KIND_TO_RECORD_LOOKUP.user?.subtitle).toEqual(['email']);
67+
for (const kind of ['team', 'department', 'position'] as const) {
68+
expect(KIND_TO_RECORD_LOOKUP[kind]?.picker).toBeUndefined();
69+
}
70+
});
71+
});

packages/app-shell/src/views/metadata-admin/inspectors/FlowReferenceField.tsx

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import {
3939
Select, SelectContent, SelectItem, SelectTrigger, SelectValue,
4040
} from '@object-ui/components';
4141
import { Pencil, Search } from 'lucide-react';
42+
import { APPROVER_VALUE_SOURCES } from '@objectstack/spec/automation';
4243
import { useAdapter } from '@object-ui/react';
4344
import { LookupField } from '@object-ui/fields';
4445
import type { FlowReferenceSpec, ReferenceKind, RefValueSource } from './flow-node-config';
@@ -100,12 +101,61 @@ export interface RecordLookupBinding {
100101
/** Secondary-line fields for people rows. */
101102
subtitle?: string[];
102103
}
103-
export const KIND_TO_RECORD_LOOKUP: Partial<Record<ReferenceKind, RecordLookupBinding>> = {
104-
user: { object: 'sys_user', valueField: 'id', displayField: 'name', picker: 'search', subtitle: ['email'] },
105-
team: { object: 'sys_team', valueField: 'id', displayField: 'name' },
106-
department: { object: 'sys_business_unit', valueField: 'id', displayField: 'name' },
107-
position: { object: 'sys_position', valueField: 'name', displayField: 'label' },
108-
};
104+
/**
105+
* PRESENTATION for the directory-backed kinds — which column to show, whether
106+
* to open the people picker, what subtitle to put under a row.
107+
*
108+
* Deliberately local: the spec publishes the data contract only and says so.
109+
* Everything below this line is objectui's call; everything above it (which
110+
* object, which column is committed) now comes from the spec.
111+
*/
112+
const RECORD_LOOKUP_PRESENTATION = {
113+
user: { displayField: 'name', picker: 'search', subtitle: ['email'] },
114+
team: { displayField: 'name' },
115+
department: { displayField: 'name' },
116+
position: { displayField: 'label' },
117+
} as const satisfies Partial<
118+
Record<ReferenceKind, Omit<RecordLookupBinding, 'object' | 'valueField'>>
119+
>;
120+
121+
/**
122+
* The older-server fallback, DERIVED from the spec's published binding.
123+
*
124+
* `object` / `valueField` used to be hand-written here, under a comment saying
125+
* this table "mirrors `APPROVER_VALUE_BINDINGS` … import it once a published
126+
* release carries the export". `@objectstack/spec@17` carries it, so this now
127+
* reads `APPROVER_VALUE_SOURCES` and composes objectui's presentation on top
128+
* (objectui#3017).
129+
*
130+
* That matters more here than anywhere else in the designer: the FIRST copy of
131+
* this data contract was wrong — every directory kind was wired to the metadata
132+
* registry, which holds no `sys_user` / `sys_team` / `sys_business_unit` /
133+
* `sys_position` ROWS, so candidates came back empty, the control degraded to
134+
* free text, and `sales_manager` got typed into a field that accepts three
135+
* values (framework #3508). Spec answered by publishing the binding with a
136+
* `satisfies` that makes an undeclared `ApproverType` a compile error; reading
137+
* it here is what finally carries that guarantee across the repo boundary.
138+
*
139+
* A kind whose spec source is not `data` is skipped rather than thrown on — a
140+
* module-level throw would white-screen the designer over a vocabulary change.
141+
* `FlowReferenceField.specDerivation.test.ts` asserts the table is non-vacuous
142+
* and still covers every kind with presentation.
143+
*/
144+
export const KIND_TO_RECORD_LOOKUP: Partial<Record<ReferenceKind, RecordLookupBinding>> =
145+
Object.fromEntries(
146+
Object.entries(RECORD_LOOKUP_PRESENTATION).flatMap(([kind, presentation]) => {
147+
const source = APPROVER_VALUE_SOURCES[kind as keyof typeof APPROVER_VALUE_SOURCES];
148+
if (!source || source.source !== 'data') return [];
149+
return [[
150+
kind,
151+
{
152+
object: source.object,
153+
valueField: source.valueField as RecordLookupBinding['valueField'],
154+
...presentation,
155+
},
156+
]];
157+
}),
158+
);
109159

110160
/**
111161
* better-auth org-membership tiers. `org-membership-level` is intentionally NOT

0 commit comments

Comments
 (0)