Skip to content

Commit d72ac48

Browse files
committed
refactor(app-shell): stop declaring 28 symbols under names the spec owns
Batch 3 of the objectstack#4115 debt burn-down (objectui#3157), and the ledger's largest single package. Twenty-eight app-shell symbols were declared under names `@objectstack/spec` already exports. Twenty are now imported or derived from the spec; eight are renamed because they model something the spec's same-named export does not. The comment justifying the largest cluster -- "kept local so app-shell does not take a build dependency on the framework spec package" -- was already false: `@objectstack/spec` is a direct dependency of this package. Three live defects the copies were hiding, each fixed by importing the type: * `SchemaDiffEntryKind` was missing `index_mismatch` / `unmapped_index` (framework#3728). ValidationPanel labels diffs from a total map, so an index divergence the server already emits arrived unlabelled. Widening the union made the compiler demand the two missing labels. * `ExplainLayer.contributors[].state` ('active' | 'expired') was absent, so an EXPIRED position / permission-set contribution rendered as a live one. * Several keys were optional locally and required in the spec (`ExternalColumn.primaryKey`, `ExplainRecordAttribution.rules`, `ExplainDecision.principal.positions` / `.permissionSets`), leaving nullish branches that can never fire. Renamed, with what the spec's same-named export actually is: FieldInput -> ScreenFieldInput (an object FIELD's authoring shape) ConversationSummary -> ConversationListItem (the AI context-compaction record) RuntimeConfig -> AppShellRuntimeConfig (the ENGINE runtime config) PageHeaderProps -> PageHeaderComponentProps (the authored SDUI header schema) FlowNode / FlowEdge -> FlowDesignerNode / ...Edge (a COMPLETE authored node/edge) PackageManifest -> PackageManifestRow (the full authored manifest) InstalledPackage -> InstalledPackageRow (the full install record) `FieldGroup` becomes `ObjectFieldGroup` -- the spec's own name for this exact shape -- and is derived from `z.input<typeof ObjectFieldGroupSchema>`, not the exported `z.infer` type: `collapse` carries `.default('none')`, so it is optional to author and required after parsing, and this designer authors. Two symbols are derived structurally with one pinned divergence each: `ScreenSpec` keeps `fields` optional (#3528) and `DecisionOutputDef` adds `required`, which the server enforces but the spec does not model yet. Deriving the latter narrowed `type` from a bare string to the spec's closed enum, so a typo'd picker kind fails to compile rather than degrading to a raw record-id text box (objectui#2955). FlowNode/FlowEdge were NOT derived on purpose: a canvas holds nodes the user has dropped but not finished (no label yet, no edge id yet), so the spec's complete-node type would make the editor's own intermediate state unrepresentable. Two genuine findings there are recorded rather than silently changed, because both alter what gets written to metadata: the designer persists geometry as `ui: {x,y}` while the spec models it twice already (`FlowNode.position`, `FlowCanvasNode`), and the edge inspector can build a `condition` object missing ADR-0089's required `dialect`. Ledger 115 -> 87 collisions; `@object-ui/app-shell` drops out entirely. Mutation-tested in three directions: re-forking a burned symbol trips the guard by name and file, reverting a rename trips it, and leaving a burned name in DEBT trips the rot ratchet. The new parity test also checks `isAggregatedViewContainer` by REFERENCE identity -- the copy it replaced was line-for-line identical, so no value comparison could have caught it (objectui#3003). Refs objectui#3157, objectstack#4115 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Jdef6ZFhJmiNRhNCd4DW3
1 parent 850033c commit d72ac48

31 files changed

Lines changed: 809 additions & 397 deletions
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
---
2+
"@object-ui/app-shell": minor
3+
---
4+
5+
Stop declaring 28 app-shell symbols under names `@objectstack/spec` owns
6+
(objectui#3157, objectstack#4115 batch 3).
7+
8+
**Breaking for importers of `@object-ui/app-shell`** — eight exported names
9+
changed, because the spec exports the same name for a *different* thing:
10+
11+
| was | now | what the spec's same-named export actually is |
12+
|:--|:--|:--|
13+
| `FieldInput` | `ScreenFieldInput` | the authoring shape of an object FIELD |
14+
| `ConversationSummary` | `ConversationListItem` | the AI context-compaction record |
15+
| `RuntimeConfig` | `AppShellRuntimeConfig` | the ENGINE runtime config |
16+
| `PageHeaderProps` | `PageHeaderComponentProps` | the authored SDUI page-header schema |
17+
| `FlowNode` / `FlowEdge` | `FlowDesignerNode` / `FlowDesignerEdge` | a COMPLETE authored flow node/edge |
18+
| `PackageManifest` | `PackageManifestRow` | the full authored package manifest |
19+
| `InstalledPackage` | `InstalledPackageRow` | the full install record |
20+
21+
The object designer's `FieldGroup` also becomes `ObjectFieldGroup` — that is
22+
the spec's own name for this exact shape, while its `FieldGroup` is the Studio
23+
field-editor's group config. The other nineteen keep their names and are now
24+
imported or derived from the spec instead of re-declared.
25+
26+
**Three live defects the copies were hiding**, all fixed by importing the real
27+
types:
28+
29+
- `SchemaDiffEntryKind` was missing `index_mismatch` and `unmapped_index`
30+
(framework#3728). The federation validate panel renders a label per kind from
31+
a total map, so an index divergence — which the server already emits — arrived
32+
as a diff row this UI could not name. The union is now the spec's, and the
33+
compiler required the two missing labels.
34+
- `ExplainLayer.contributors[].state` (`'active' | 'expired'`) did not exist in
35+
the local copy of the access-explain report, so an EXPIRED permission-set or
36+
position contribution rendered identically to a live one.
37+
- `ExternalColumn.primaryKey` was optional locally while the server always sends
38+
it (the spec schema defaults it), and `ExplainRecordAttribution.rules` /
39+
`ExplainDecision.principal.positions` / `.permissionSets` were optional here
40+
and required there — every reader carried a nullish branch that could not fire.
41+
42+
The comment justifying the largest copy ("kept local so app-shell does not take
43+
a build dependency on the framework spec package") was already false:
44+
`@objectstack/spec` is a direct dependency of this package.
45+
46+
Two symbols are derived structurally rather than re-exported, each with one
47+
documented divergence pinned by a test: `ScreenSpec` keeps `fields` optional
48+
(an `object-form` step legitimately sends none — #3528), and `DecisionOutputDef`
49+
adds `required`, which the server enforces but the spec does not yet model.
50+
Deriving the latter also narrowed its `type` from a bare `string` to the spec's
51+
closed enum, so a typo'd picker kind now fails to compile instead of silently
52+
degrading to a raw record-id text box (objectui#2955).
Lines changed: 286 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,286 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* app-shell ↔ `@objectstack/spec` symbol-collision tripwires
11+
* (objectui#3157, objectstack#4115 burn-down batch 3).
12+
*
13+
* Twenty-eight app-shell symbols used to be declared under names the spec
14+
* already owns. Twenty were burned down by importing or deriving the spec's own
15+
* type (eighteen plain re-exports, plus `ScreenSpec` and `DecisionOutputDef`
16+
* derived structurally with one documented divergence each); eight were renamed
17+
* because they model something the spec's same-named export does not.
18+
*
19+
* One symbol is in both camps: the object designer's `FieldGroup` was renamed to
20+
* `ObjectFieldGroup` AND derived — the spec owns that exact shape, just under
21+
* the other name, while its `FieldGroup` is the Studio field-editor's group
22+
* config. Renaming to the spec's own name was the fix.
23+
*
24+
* A rename only stays a fix for as long as the new name is genuinely free. If
25+
* the spec later ships a `FlowDesignerNode`, this package would quietly be back
26+
* where it started — a local declaration under a spec export's name, read by
27+
* the next agent as the spec's own definition. These tests are that tripwire.
28+
*
29+
* ## Why the spec's names are read through the compiler, not `import * as`
30+
*
31+
* A runtime namespace import sees VALUES only, and almost every symbol in this
32+
* burn-down is a TYPE (`FieldInput`, `RuntimeConfig`, `ConversationSummary`, …).
33+
* A tripwire built on `Object.keys(await import('@objectstack/spec/ui'))` would
34+
* pass for every one of them while proving nothing — the same mistake the
35+
* guard's own header records having made in its first draft. So this reads each
36+
* subpath's `.d.ts` through the TypeScript checker, exactly as
37+
* `scripts/check-spec-symbol-derivation.mjs` does, and gets types and values
38+
* alike.
39+
*/
40+
41+
import { describe, it, expect } from 'vitest';
42+
import ts from 'typescript';
43+
import { createRequire } from 'node:module';
44+
import { readFileSync } from 'node:fs';
45+
import { resolve, dirname } from 'node:path';
46+
47+
import { isAggregatedViewContainer } from '../views/metadata-admin/view-item-normalize';
48+
49+
import type { ScreenSpec } from '../views/ScreenView';
50+
import type { DecisionOutputDef } from '../utils/decisionOutputParams';
51+
import type { ObjectFieldGroup } from '../views/metadata-admin/previews/object-fields-io';
52+
import type {
53+
ScreenSpec as SpecScreenSpec,
54+
ScreenFieldSpec as SpecScreenFieldSpec,
55+
} from '@objectstack/spec/contracts';
56+
import type { DecisionOutputDef as SpecDecisionOutputDef } from '@objectstack/spec/automation';
57+
58+
/** Every name `@objectstack/spec` exports from any subpath — types AND values. */
59+
function specExportNames(): Set<string> {
60+
const require = createRequire(import.meta.url);
61+
const pkgPath = require.resolve('@objectstack/spec/package.json');
62+
const pkgDir = dirname(pkgPath);
63+
const pkg = JSON.parse(readFileSync(pkgPath, 'utf8')) as {
64+
exports?: Record<string, { import?: { types?: string }; require?: { types?: string } }>;
65+
};
66+
67+
const files: string[] = [];
68+
for (const cond of Object.values(pkg.exports ?? {})) {
69+
if (typeof cond !== 'object' || cond === null) continue;
70+
const dts = cond.import?.types ?? cond.require?.types;
71+
if (dts) files.push(resolve(pkgDir, dts));
72+
}
73+
74+
const program = ts.createProgram(files, {
75+
noEmit: true,
76+
skipLibCheck: true,
77+
strict: false,
78+
target: ts.ScriptTarget.ESNext,
79+
module: ts.ModuleKind.ESNext,
80+
moduleResolution: ts.ModuleResolutionKind.Bundler,
81+
});
82+
const checker = program.getTypeChecker();
83+
84+
const names = new Set<string>();
85+
for (const file of files) {
86+
const sf = program.getSourceFile(file);
87+
if (!sf) continue;
88+
const moduleSymbol = checker.getSymbolAtLocation(sf);
89+
if (!moduleSymbol) continue;
90+
for (const exported of checker.getExportsOfModule(moduleSymbol)) names.add(exported.getName());
91+
}
92+
return names;
93+
}
94+
95+
const SPEC_NAMES = specExportNames();
96+
97+
/**
98+
* Sanity: if this set came back empty (bad resolve, changed `exports` map), every
99+
* "the spec does not own X" assertion below would pass vacuously.
100+
*/
101+
describe('the spec export-name probe itself works', () => {
102+
it('reads a non-trivial number of names', () => {
103+
expect(SPEC_NAMES.size).toBeGreaterThan(1000);
104+
});
105+
106+
it('sees TYPE-only exports, not just runtime values', () => {
107+
// `FieldInput` is `Omit<Partial<Field>, 'type'>` — invisible to `import()`.
108+
expect(SPEC_NAMES.has('FieldInput')).toBe(true);
109+
});
110+
});
111+
112+
/**
113+
* The ten renames. Each entry is `[local dialect name, the spec name it used to
114+
* collide with]`, with a one-line note on what the spec's symbol actually means
115+
* — the thing the old name falsely claimed.
116+
*/
117+
const RENAMES: Array<[local: string, formerly: string, specMeaning: string]> = [
118+
['ScreenFieldInput', 'FieldInput', "the authoring shape of an object FIELD (Omit<Partial<Field>, 'type'>)"],
119+
['ConversationListItem', 'ConversationSummary', 'the AI context-COMPACTION record (keyPoints, tokensSaved, …)'],
120+
['AppShellRuntimeConfig', 'RuntimeConfig', 'the ENGINE runtime config (engine, engineConfig, resourceLimits)'],
121+
['PageHeaderComponentProps', 'PageHeaderProps', 'the AUTHORED SDUI page-header node schema (strings, action ids)'],
122+
['FlowDesignerNode', 'FlowNode', 'a COMPLETE authored flow node (label required)'],
123+
['FlowDesignerEdge', 'FlowEdge', 'a COMPLETE authored flow edge (id required, condition needs `dialect`)'],
124+
['PackageManifestRow', 'PackageManifest', 'the full authored package manifest (~40 keys)'],
125+
['InstalledPackageRow', 'InstalledPackage', 'the full install record (installedAt, upgradeHistory, …)'],
126+
];
127+
128+
describe('renamed local dialects do not collide with a spec export', () => {
129+
it.each(RENAMES)('the spec does not own `%s`', (local) => {
130+
expect(
131+
SPEC_NAMES.has(local),
132+
`@objectstack/spec now exports \`${local}\`. This package declares its own ` +
133+
`\`${local}\`, so the rename that fixed objectstack#4115 has re-created the ` +
134+
`collision under the new name. Rename again (and check the new name here ` +
135+
`FIRST — objectui#3074 landed a rename onto another spec export exactly ` +
136+
`this way), or derive from the spec if the two really are the same thing.`,
137+
).toBe(false);
138+
});
139+
140+
/**
141+
* The other half of the ratchet. If the spec ever RETIRES the name that forced
142+
* a rename, the rename is no longer load-bearing and the local dialect can go
143+
* back to the natural name — this fails and says so, so the workaround cannot
144+
* outlive its reason.
145+
*/
146+
it.each(RENAMES)('the spec still owns `%s` (second value: %s)', (_local, formerly) => {
147+
expect(
148+
SPEC_NAMES.has(formerly),
149+
`@objectstack/spec no longer exports \`${formerly}\`, which is the only ` +
150+
`reason this package renamed it. Either the spec dropped it (then take the ` +
151+
`plain name back) or it moved (then re-check what it means now).`,
152+
).toBe(true);
153+
});
154+
});
155+
156+
/**
157+
* `FlowCanvasNode` / `FlowCanvasEdge` are the names one would naturally reach for
158+
* when renaming the designer's node/edge types. They are already spec exports —
159+
* and they mean the pure VISUAL OVERLAY (`{ nodeId, x, y, collapsed, … }`), not
160+
* the node. Pinned so a future rename does not walk into them.
161+
*/
162+
describe('the obvious alternative flow names are already taken', () => {
163+
it.each(['FlowCanvasNode', 'FlowCanvasEdge'])('`%s` belongs to the spec', (name) => {
164+
expect(SPEC_NAMES.has(name)).toBe(true);
165+
});
166+
});
167+
168+
/**
169+
* Re-exports must be the spec's own binding, not a copy that happens to agree.
170+
* Reference identity is the only check that can tell those apart — a faithful
171+
* copy passes every value comparison (objectui#3003).
172+
*/
173+
describe('re-exported values are the spec binding itself', () => {
174+
it('isAggregatedViewContainer IS the spec function', async () => {
175+
const spec = await import('@objectstack/spec');
176+
expect(isAggregatedViewContainer).toBe(spec.isAggregatedViewContainer);
177+
});
178+
179+
it('still behaves as the metadata list needs', () => {
180+
expect(isAggregatedViewContainer({ list: {} })).toBe(true);
181+
expect(isAggregatedViewContainer({ listViews: {} })).toBe(true);
182+
// An already-expanded ViewItem carries the discriminant and is NOT a container.
183+
expect(isAggregatedViewContainer({ viewKind: 'list', list: {} })).toBe(false);
184+
expect(isAggregatedViewContainer({ name: 'x' })).toBe(false);
185+
expect(isAggregatedViewContainer(null)).toBe(false);
186+
});
187+
});
188+
189+
/* -------------------------------------------------------------------------- */
190+
/* Structural derivations — the three symbols that are neither a plain */
191+
/* re-export nor a rename. Each pins its ONE documented divergence, so the */
192+
/* divergence cannot silently grow and cannot silently outlive its reason. */
193+
/* -------------------------------------------------------------------------- */
194+
195+
/** Compile-time assertions. A violation is a `tsc` error, not a runtime failure. */
196+
type Assert<T extends true> = T;
197+
type Extends<A, B> = [A] extends [B] ? true : false;
198+
type IsAny<T> = 0 extends 1 & T ? true : false;
199+
type Equal<A, B> = (<T>() => T extends A ? 1 : 2) extends <T>() => T extends B ? 1 : 2
200+
? true
201+
: false;
202+
203+
describe('ScreenSpec derives from the spec, widening only `fields`', () => {
204+
it('is pinned at compile time', () => {
205+
// Guard against the probe lying: if either side erased to `any`, every
206+
// assignability assertion below would pass while proving nothing
207+
// (objectstack#4171 is exactly that failure for other symbols).
208+
type _NotAny = Assert<Equal<IsAny<SpecScreenSpec>, false>>;
209+
type _LocalNotAny = Assert<Equal<IsAny<ScreenSpec>, false>>;
210+
211+
// The spec's screen is always a valid local screen: widening only ever adds.
212+
type _SpecIsUsableHere = Assert<Extends<SpecScreenSpec, ScreenSpec>>;
213+
214+
// …but not the reverse, and for exactly one reason: `fields` is optional
215+
// here. If this ever becomes `true`, the spec has made `fields` optional
216+
// itself and this alias should collapse to a plain re-export.
217+
type _StillWidened = Assert<Equal<Extends<ScreenSpec, SpecScreenSpec>, false>>;
218+
219+
// The widening is confined to `fields` — every other key is the spec's.
220+
type _OnlyFieldsDiffers = Assert<
221+
Extends<Omit<ScreenSpec, 'fields'>, Omit<SpecScreenSpec, 'fields'>>
222+
>;
223+
type _FieldsIsSpecFields = Assert<
224+
Equal<NonNullable<ScreenSpec['fields']>, SpecScreenFieldSpec[]>
225+
>;
226+
227+
// No key was invented locally, and none of the spec's was dropped.
228+
type _NoLocalOnlyKeys = Assert<Equal<Exclude<keyof ScreenSpec, keyof SpecScreenSpec>, never>>;
229+
type _NoMissingKeys = Assert<Equal<Exclude<keyof SpecScreenSpec, keyof ScreenSpec>, never>>;
230+
231+
expect(true).toBe(true);
232+
});
233+
});
234+
235+
describe('DecisionOutputDef derives from the spec, adding only `required`', () => {
236+
it('is pinned at compile time', () => {
237+
type _NotAny = Assert<Equal<IsAny<SpecDecisionOutputDef>, false>>;
238+
239+
// Every spec decision output is usable here.
240+
type _SpecIsUsableHere = Assert<Extends<SpecDecisionOutputDef, DecisionOutputDef>>;
241+
242+
// `required` is the ONLY local addition. When the spec adopts it, this
243+
// becomes `never`, the assertion fails, and the interface should collapse
244+
// to a plain re-export.
245+
type _OnlyRequiredAdded = Assert<
246+
Equal<Exclude<keyof DecisionOutputDef, keyof SpecDecisionOutputDef>, 'required'>
247+
>;
248+
249+
// Deriving NARROWED `type` from the bare `string` this file used to declare
250+
// to the spec's closed enum — that narrowing is the point, so pin it.
251+
type _TypeIsClosed = Assert<
252+
Equal<DecisionOutputDef['type'], 'user' | 'department' | 'position' | 'team' | 'text' | undefined>
253+
>;
254+
255+
expect(true).toBe(true);
256+
});
257+
});
258+
259+
describe('ObjectFieldGroup derives from the spec schema INPUT side', () => {
260+
it('keeps `collapse` authorable (the z.input vs z.infer trap)', () => {
261+
// `collapse` carries `.default('none')`, so it is optional to AUTHOR and
262+
// required after parsing. This designer authors — `addGroup` emits
263+
// `{ key, label }` — so the output type would make its own new-group shape
264+
// unrepresentable. If this flips, someone swapped z.input for z.infer.
265+
type _CollapseOptional = Assert<Extends<{ key: string; label: string }, ObjectFieldGroup>>;
266+
267+
// Still the real spec vocabulary, not a hand copy that merely agrees.
268+
type _HasSpecKeys = Assert<
269+
Extends<
270+
'key' | 'label' | 'icon' | 'description' | 'collapse' | 'collapsible' | 'collapsed' | 'defaultExpanded',
271+
keyof ObjectFieldGroup
272+
>
273+
>;
274+
type _NoInventedKeys = Assert<
275+
Equal<
276+
Exclude<
277+
keyof ObjectFieldGroup,
278+
'key' | 'label' | 'icon' | 'description' | 'collapse' | 'collapsible' | 'collapsed' | 'defaultExpanded'
279+
>,
280+
never
281+
>
282+
>;
283+
284+
expect(true).toBe(true);
285+
});
286+
});

packages/app-shell/src/console/ai/ConversationsSidebar.tsx

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {
2020
cn,
2121
} from '@object-ui/components';
2222
import { agentAliasGroup, agentRouteName } from '@object-ui/plugin-chatbot';
23-
import { useConversationList, type ConversationSummary } from '../../hooks/useConversationList';
23+
import { useConversationList, type ConversationListItem } from '../../hooks/useConversationList';
2424

2525
export interface ConversationsSidebarProps {
2626
userId: string | undefined;
@@ -73,7 +73,7 @@ export type ConversationGroupKey = 'today' | 'yesterday' | 'previous7Days' | 'pr
7373

7474
export interface ConversationGroup {
7575
key: ConversationGroupKey;
76-
items: ConversationSummary[];
76+
items: ConversationListItem[];
7777
}
7878

7979
const GROUP_ORDER: ConversationGroupKey[] = ['today', 'yesterday', 'previous7Days', 'previous30Days', 'older'];
@@ -95,18 +95,18 @@ export const CONVERSATION_GROUP_LABELS: Record<ConversationGroupKey, string> = {
9595
* component's render path). Empty sections are omitted.
9696
*/
9797
export function groupConversationsByDate(
98-
conversations: ConversationSummary[],
98+
conversations: ConversationListItem[],
9999
nowMs: number = Date.now(),
100100
): ConversationGroup[] {
101101
const startOfToday = new Date(nowMs);
102102
startOfToday.setHours(0, 0, 0, 0);
103103
const todayMs = startOfToday.getTime();
104104
const DAY = 24 * 60 * 60 * 1000;
105-
const stamp = (c: ConversationSummary): number => {
105+
const stamp = (c: ConversationListItem): number => {
106106
const v = new Date(c.updatedAt ?? c.createdAt ?? 0).getTime();
107107
return Number.isNaN(v) ? 0 : v;
108108
};
109-
const buckets: Record<ConversationGroupKey, ConversationSummary[]> = {
109+
const buckets: Record<ConversationGroupKey, ConversationListItem[]> = {
110110
today: [],
111111
yesterday: [],
112112
previous7Days: [],
@@ -185,7 +185,7 @@ export function ConversationsSidebar({
185185
// Navigate to a conversation on its OWN agent surface (so a lenient
186186
// cross-agent row still opens correctly); fall back to this surface.
187187
const conversationHref = useCallback(
188-
(c: ConversationSummary) => {
188+
(c: ConversationListItem) => {
189189
const seg = c.agentId ? agentRouteName(c.agentId) : agentRoute;
190190
return seg ? `/ai/${seg}/${c.id}` : `/ai/${c.id}`;
191191
},
@@ -305,7 +305,7 @@ export function ConversationsSidebar({
305305
}
306306

307307
interface RowProps {
308-
conversation: ConversationSummary;
308+
conversation: ConversationListItem;
309309
/** Active search query — matched substrings are highlighted in title/preview. */
310310
query?: string;
311311
active: boolean;

0 commit comments

Comments
 (0)