Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions .changeset/6287-flownode-description-key.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
---
'@object-ui/app-shell': patch
---

The flow designer's node inspector no longer offers a **Description** field, and strips a
stored `description` off a node the first time an author edits it (objectui#6287).

`FlowNodeSchema` is `.strict()` (objectstack#4001) and refuses that key by name — measured
on the installed `@objectstack/spec@17.2.0`:

```
FlowNodeSchema.safeParse({ id, type, label, description, config })
-> unrecognized_keys: ["description"]
```

By this package's own reading of that mechanism, the cost is not untidiness but an
unsavable draft: the key "surfaces as `unrecognized_keys` in the live client validation and
as a 422 on save" (`flow-canvas-layout.withCanonicalGeometry`, on the identical retired `ui`
case). So the field was not merely describing a shape the contract refuses — it was
producing one, on every keystroke, and nothing anywhere read the value back. The spec's flow
node has eleven keys and no note key of any spelling, so there was no reader to grow into.

Stored flows heal on the author's first edit, the same migrate-on-write boundary the retired
`ui` geometry gets, and for the same reason: with the field gone there would otherwise be no
way left to clear a `description` an author had already saved.

The three hand-written copies of the node and edge shapes that let this drift go unseen are
now one declaration each — `FlowNodeInspector`'s node and edge types alias the canonical
`FlowNodeLike` / `FlowDesignerEdge`, and `flow-decision-edges`' fourth edge copy aliases the
same canvas edge instead of restating it with a `condition?: unknown` that had already
outlived objectui#3202's narrowing by months.
2 changes: 0 additions & 2 deletions packages/app-shell/src/views/metadata-admin/i18n.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -391,7 +391,6 @@ const ENGINE_STRINGS_EN: Record<string, string> = {
'engine.inspector.flowNode.id': 'ID',
'engine.inspector.flowNode.label': 'Label',
'engine.inspector.flowNode.type': 'Node Type',
'engine.inspector.flowNode.description': 'Description',
'engine.inspector.flowNode.configuration': 'Configuration',
'engine.inspector.flowNode.config': 'Config (JSON)',
'engine.inspector.flowNode.advanced': 'Advanced (JSON)',
Expand DownExpand Up@@ -2210,7 +2209,6 @@ const ENGINE_STRINGS_ZH: Record<string, string> = {
'engine.inspector.flowNode.id': 'ID',
'engine.inspector.flowNode.label': '标签',
'engine.inspector.flowNode.type': '节点类型',
'engine.inspector.flowNode.description': '描述',
'engine.inspector.flowNode.configuration': '配置',
'engine.inspector.flowNode.config': '配置(JSON)',
'engine.inspector.flowNode.advanced': '高级(JSON)',
Expand Down
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.

/**
* **FlowNodeInspector writes only keys `FlowNodeSchema` accepts** (objectui#6287).
*
* The inspector used to offer a "Description" text field that committed
* `node.description`. `FlowNodeSchema` is `.strict()` (objectstack#4001) and
* refuses that key BY NAME:
*
* FlowNodeSchema.safeParse({ id, type, label, description, config })
* -> unrecognized_keys: ["description"]
*
* By this package's own established reading of that mechanism, the cost is not
* untidiness — it is an unsavable draft: "the key surfaces as
* `unrecognized_keys` in the live client validation and as a 422 on save"
* (`flow-canvas-layout.withCanonicalGeometry`, on the identical `ui` case).
* So the field did not merely describe a shape the contract refuses, it
* PRODUCED one, on every keystroke.
*
* ## Why the obvious pins are ghosts, and what this file pins instead
*
* Dropping `description?` from a local `interface` proves nothing on its own,
* for two independent reasons measured on this card:
*
* 1. The node the inspector edits is typed `FlowNodeLike` (the exported shape
* `locateFlowNode` returns), NOT the inspector's own module-local
* declaration — so narrowing only the local copy changes no read.
* 2. Both shapes carry a deliberately load-bearing `[k: string]: unknown`
* index signature (the canvas round-trips node properties it does not
* itself understand, and dropping them on save would be data loss). An
* index signature absorbs every excess property, so an object literal
* carrying `description` type-checks in BOTH worlds: a `@ts-expect-error`
* negative test does not go red before the fix, it goes red AFTER it, as an
* unused directive. Measured, not assumed.
*
* The two assertions below survive both traps:
*
* - **Compile time**: the DECLARED members of `FlowNodeLike` — index signature
* stripped — must be a subset of the spec's own `FlowNode` keys. That closes
* the whole class rather than the one key: any future member added to the
* read type that the contract refuses turns this red.
* - **Runtime**: what the inspector actually EMITS must parse clean through the
* real `FlowNodeSchema`, including for a stored node that already carries
* `description` — which heals on the author's first edit, exactly as the
* retired `ui` geometry does.
*
* Both are guarded against lying: the type assertions carry `IsAny` /
* non-empty-key-set probes (a degenerate probe passes every assignability test
* while proving nothing — objectstack#4171), and the runtime assertions carry a
* positive control proving this really is the strict schema and that the
* queries used to prove a control ABSENT can find controls that are present.
*/

import { describe, it, expect, vi, afterEach } from 'vitest';
import { render, screen, cleanup, fireEvent } from '@testing-library/react';
import * as Automation from '@objectstack/spec/automation';
import type { FlowNode as SpecFlowNode } from '@objectstack/spec/automation';
import type { FlowNodeLike } from './flow-nested-selection';

// Same stubs the sibling suite uses: the engine config-schema hook is empty so
// the hardcoded field groups render, and the field catalog resolves without a
// network client.
vi.mock('../previews/useFlowNodePalette', () => ({
useActionConfigSchemas: () => ({}),
useFlowNodePalette: () => [],
}));
vi.mock('../previews/useObjectFields', () => ({
useObjectFields: () => ({ fields: [], loading: false, error: null }),
}));

import { FlowNodeInspector } from './FlowNodeInspector';
import type { MetadataSelection } from '../preview-registry';

afterEach(cleanup);

interface ZodIssue {
code?: string;
keys?: string[];
path: PropertyKey[];
message: string;
}
interface ZodLike {
safeParse: (value: unknown) => { success: boolean; error?: { issues: ZodIssue[] } };
}
const spec = Automation as unknown as Record<string, ZodLike | undefined>;
const FlowNodeSchema = spec.FlowNodeSchema!;

const explain = (r: { success: boolean; error?: { issues: ZodIssue[] } }) =>
r.success ? '' : JSON.stringify(r.error?.issues ?? [], null, 1);

/**
* A draft whose first node ALREADY carries the refused key — the state an
* author reached with the old Description field, and the one that must heal.
*/
function makeDraft() {
return {
nodes: [
{ id: 'greet', type: 'screen', label: 'Greet', description: 'says hello', config: { title: 'Hi' } },
{ id: 'done', type: 'end', label: 'Done' },
],
edges: [{ source: 'greet', target: 'done' }],
};
}

function renderInspector(selection: MetadataSelection, draft: Record<string, unknown> = makeDraft()) {
const onPatch = vi.fn();
const utils = render(
<FlowNodeInspector
type="flow"
name="welcome"
draft={draft}
selection={selection}
onPatch={onPatch}
onClearSelection={vi.fn()}
readOnly={false}
locale="en-US"
/>,
);
return { onPatch, ...utils };
}

const lastPatch = (onPatch: ReturnType<typeof vi.fn>) => onPatch.mock.calls.at(-1)![0] as any;

type Assert<T extends true> = T;
type Extends<A, B> = [A] extends [B] ? true : false;
type IsAny<T> = 0 extends 1 & T ? true : false;
type Equal<A, B> = (<T>() => T extends A ? 1 : 2) extends <T>() => T extends B ? 1 : 2 ? true : false;

/**
* The DECLARED members of a type — its index signature removed.
*
* `keyof FlowNodeLike` is `string | number` while the index signature is there,
* which is why the naive key comparison cannot see this defect at all.
*/
type Declared<T> = {
[K in keyof T as string extends K ? never : number extends K ? never : K]: T[K];
};

describe('the node read type declares no key FlowNodeSchema refuses (#6287)', () => {
it('is pinned at compile time', () => {
type DeclaredNodeKeys = keyof Declared<FlowNodeLike>;
type SpecNodeKeys = keyof SpecFlowNode;

// Guard against a degenerate probe: were either side `any`, or the
// index-signature strip to leave nothing behind, every assertion below
// would pass while measuring nothing.
type _SpecNotAny = Assert<Equal<IsAny<SpecFlowNode>, false>>;
type _LocalNotAny = Assert<Equal<IsAny<FlowNodeLike>, false>>;
type _StripLeftKeys = Assert<Equal<Equal<DeclaredNodeKeys, never>, false>>;
type _SpecHasKeys = Assert<Extends<'label', SpecNodeKeys>>;
// …and that the strip really removed the index signature: `description`
// is assignable to the RAW type in both worlds (that is the ghost), so a
// comparison that still saw the index signature could never go red.
type _IndexSignatureGone = Assert<Equal<Extends<'anyStringAtAll', DeclaredNodeKeys>, false>>;

// THE PIN. Not "is compatible with" — no declared member may be a key the
// strict contract refuses. `description` is what made this red.
type _NoRefusedKey = Assert<Equal<Exclude<DeclaredNodeKeys, SpecNodeKeys>, never>>;

expect(true).toBe(true);
});
});

describe('what FlowNodeInspector emits parses through the real FlowNodeSchema (#6287)', () => {
it('the schema under test is the strict one that refuses `description`', () => {
// Blind-instrument guard: if this parse ever SUCCEEDS, every assertion
// below is vacuous and the premise of #6287 has moved.
const refused = FlowNodeSchema.safeParse({ id: 'greet', type: 'screen', label: 'Greet', description: 'x' });
expect(refused.success).toBe(false);
const issue = refused.error!.issues.find((i) => i.code === 'unrecognized_keys');
expect(issue, explain(refused)).toBeDefined();
expect(issue!.keys).toContain('description');
// …and that the same node WITHOUT it parses, so the refusal is about the
// key and not about the rest of the fixture.
expect(FlowNodeSchema.safeParse({ id: 'greet', type: 'screen', label: 'Greet' }).success).toBe(true);
});

it('offers no control that writes a key the contract refuses', () => {
renderInspector({ kind: 'node', id: 'greet' });
// Positive control for the query itself: the fields that SHOULD be there
// are found by the same lookup that must come up empty below.
expect(screen.getByLabelText('Label')).toBeInTheDocument();
expect(screen.getByLabelText('ID')).toBeInTheDocument();
expect(screen.queryByLabelText('Description')).toBeNull();
});

it('heals a stored node that already carries `description` on the first edit', () => {
const { onPatch } = renderInspector({ kind: 'node', id: 'greet' });
fireEvent.change(screen.getByDisplayValue('Greet'), { target: { value: 'Greet the user' } });
const node = lastPatch(onPatch).nodes[0];
expect(node.label).toBe('Greet the user'); // the edit itself landed
expect(node.config).toEqual({ title: 'Hi' }); // unrelated keys survive
expect('description' in node).toBe(false);
const parsed = FlowNodeSchema.safeParse(node);
expect(parsed.success, explain(parsed)).toBe(true);
});

it('heals through the config-field write path too', () => {
const { onPatch } = renderInspector({ kind: 'node', id: 'greet' });
// The screen node's `title` config field — a different write path
// (`setField` -> loc.write) than the top-level label edit above.
fireEvent.change(screen.getByDisplayValue('Hi'), { target: { value: 'Hello' } });
const node = lastPatch(onPatch).nodes[0];
expect(node.config.title).toBe('Hello');
const parsed = FlowNodeSchema.safeParse(node);
expect(parsed.success, explain(parsed)).toBe(true);
});
});
Original file line numberDiff line numberDiff line change
Expand Up@@ -10,8 +10,8 @@
* `draft.nodes[i]`; nested nodes rebuild the container's
* `config.<region>.nodes[i]` with explicit spreads.
*
* Both share the SAME schema-driven form. Beyond id / label / type /
* description, each node type exposes a set of typed form fields (see
* Both share the SAME schema-driven form. Beyond id / label / type, each node
* type exposes a set of typed form fields (see
* `flow-node-config`, or the engine-published configSchema) that edit scalar
* keys on `node.config`; remaining keys go to an "Advanced (JSON)" block so
* authors are never locked out. A nested node is edit-only this phase: its id is
Expand DownExpand Up@@ -54,27 +54,61 @@ import { useActionConfigSchemas } from '../previews/useFlowNodePalette.js';
import { FlowNodeConfigField } from './FlowNodeConfigField.js';
import { useFlowScope } from './useFlowScope.js';
import { nodeOutputRefs, type ScopeRef } from './flow-scope.js';
import { NESTED_NODE_KIND, parseNestedNodeId, locateFlowNode } from './flow-nested-selection.js';
import { NESTED_NODE_KIND, parseNestedNodeId, locateFlowNode, type FlowNodeLike } from './flow-nested-selection.js';
import type { FlowDesignerEdge } from '../previews/flow-canvas-layout.js';
import { ScreenPreview } from '../previews/ScreenPreview.js';

interface FlowNode {
id: string;
type?: string;
label?: string;
description?: string;
config?: Record<string, unknown>;
[k: string]: unknown;
}
/**
* The node and edge shapes this panel edits — ALIASED, never restated
* (objectui#6287).
*
* Both used to be hand-written copies here, and both had already drifted from
* the declarations they duplicate:
*
* - the node copy declared `description?: string`, a key `FlowNodeSchema`
* refuses by name (`.strict()`, objectstack#4001) — and the copy was not even
* the type the panel reads through, since `locateFlowNode` returns
* `FlowNodeLike`. Narrowing the copy alone would have changed nothing.
* - the edge copy still spelled `condition?: unknown`, months after
* `FlowEdgeInspector`'s twin was narrowed to the spec's `ExpressionInput`
* because the loose spelling described an envelope the server rejects — the
* over-wide read type that got objectui#3171 filed against a defect that does
* not reproduce (objectui#3202).
*
* `FlowPreview.tsx` made exactly this move for its own pair, and its comment is
* about this one: "two copies of one shape is how the wrong one survives being
* fixed". Aliasing costs nothing at runtime — both imports are `import type`,
* erased at compile time, and `flow-canvas-layout` is dependency-free by design.
* `flow-designer-edge.types.test.ts` already pins the edge condition against the
* spec, so this panel now inherits that pin instead of needing its own copy of
* it.
*/
type FlowNode = FlowNodeLike;
type FlowEdge = FlowDesignerEdge;

interface FlowEdge {
id?: string;
source: string;
target: string;
condition?: unknown;
label?: string;
isDefault?: boolean;
type?: string;
[k: string]: unknown;
/**
* Node keys the spec's `.strict()` `FlowNodeSchema` refuses, stripped on write
* so a stored flow HEALS on the author's first edit.
*
* This is the same migrate-on-write boundary `withCanonicalGeometry` gives the
* retired `ui` geometry, for the same reason it gives it: a node carrying such
* a key is unsavable rather than untidy — `unrecognized_keys` in the live
* client validation, a 422 on save. `description` reached stored flows through
* this panel's own Description field (removed in objectui#6287), and with that
* field gone there would otherwise be no way left to clear it by hand.
*
* Deliberately a NAMED list rather than "everything the spec does not list":
* the index signature on the node type is load-bearing — the canvas
* round-trips node properties this layer does not understand, and a blanket
* strip would be exactly the data loss that type exists to prevent.
*/
const SPEC_REFUSED_NODE_KEYS = ['description'] as const;

function withoutSpecRefusedKeys(node: Record<string, unknown>): Record<string, unknown> {
if (!SPEC_REFUSED_NODE_KEYS.some((k) => k in node)) return node;
const next = { ...node };
for (const k of SPEC_REFUSED_NODE_KEYS) delete next[k];
return next;
}

/**
Expand DownExpand Up@@ -249,7 +283,7 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection,
}

const patchNode = (updates: Partial<FlowNode>) => {
const patch = loc?.write({ ...node, ...updates });
const patch = loc?.write(withoutSpecRefusedKeys({ ...node, ...updates }));
if (patch) onPatch(patch);
};

Expand DownExpand Up@@ -299,7 +333,7 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection,
// Migrate-on-edit: writing the canonical path drops any looser fallback
// location, so the node never carries a stale duplicate (engine + designer agree).
if (field.fallbackPath) nextNode = setAtPath(nextNode, field.fallbackPath, undefined);
const patch = loc.write(nextNode);
const patch = loc.write(withoutSpecRefusedKeys(nextNode));
if (!patch) return;
if (nextEdges) patch.edges = nextEdges;
onPatch(patch);
Expand All@@ -320,7 +354,7 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection,
const nextNode: Record<string, unknown> = { ...node };
if (Object.keys(merged).length === 0) delete nextNode.config;
else nextNode.config = merged;
const patch = loc?.write(nextNode);
const patch = loc?.write(withoutSpecRefusedKeys(nextNode));
if (patch) onPatch(patch);
} catch (e) {
setAdvError(String((e as Error).message));
Expand DownExpand Up@@ -370,13 +404,6 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection,
onCommit={(v) => patchNode({ type: v })}
disabled={readOnly}
/>
<InspectorTextField
label={t('engine.inspector.flowNode.description', locale)}
value={node.description ?? ''}
onCommit={(v) => patchNode({ description: v || undefined })}
disabled={readOnly}
/>

{fields.length === 0 ? (
<p className="pt-1 text-xs italic text-muted-foreground">
{t('engine.inspector.flowNode.noConfig', locale)}
Expand Down
Loading
Loading