diff --git a/.changeset/6287-flownode-description-key.md b/.changeset/6287-flownode-description-key.md new file mode 100644 index 0000000000..e14fbd707d --- /dev/null +++ b/.changeset/6287-flownode-description-key.md @@ -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. diff --git a/packages/app-shell/src/views/metadata-admin/i18n.ts b/packages/app-shell/src/views/metadata-admin/i18n.ts index b8f32130c5..97fbcee723 100644 --- a/packages/app-shell/src/views/metadata-admin/i18n.ts +++ b/packages/app-shell/src/views/metadata-admin/i18n.ts @@ -391,7 +391,6 @@ const ENGINE_STRINGS_EN: Record = { '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)', @@ -2210,7 +2209,6 @@ const ENGINE_STRINGS_ZH: Record = { '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）', diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.specKeys.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.specKeys.test.tsx new file mode 100644 index 0000000000..7afa48a7aa --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.specKeys.test.tsx @@ -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; +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 = makeDraft()) { + const onPatch = vi.fn(); + const utils = render( + , + ); + return { onPatch, ...utils }; +} + +const lastPatch = (onPatch: ReturnType) => onPatch.mock.calls.at(-1)![0] as any; + +type Assert = T; +type Extends = [A] extends [B] ? true : false; +type IsAny = 0 extends 1 & T ? true : false; +type Equal = (() => T extends A ? 1 : 2) extends () => 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 = { + [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; + 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, false>>; + type _LocalNotAny = Assert, false>>; + type _StripLeftKeys = Assert, false>>; + type _SpecHasKeys = Assert>; + // …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, 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, 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); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx index 75bfaca4c3..0ff581207b 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx @@ -10,8 +10,8 @@ * `draft.nodes[i]`; nested nodes rebuild the container's * `config..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 @@ -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; - [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): Record { + 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; } /** @@ -249,7 +283,7 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, } const patchNode = (updates: Partial) => { - const patch = loc?.write({ ...node, ...updates }); + const patch = loc?.write(withoutSpecRefusedKeys({ ...node, ...updates })); if (patch) onPatch(patch); }; @@ -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); @@ -320,7 +354,7 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, const nextNode: Record = { ...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)); @@ -370,13 +404,6 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, onCommit={(v) => patchNode({ type: v })} disabled={readOnly} /> - patchNode({ description: v || undefined })} - disabled={readOnly} - /> - {fields.length === 0 ? (

{t('engine.inspector.flowNode.noConfig', locale)} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/flow-decision-edges.ts b/packages/app-shell/src/views/metadata-admin/inspectors/flow-decision-edges.ts index b485bd28d3..96cc52d323 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/flow-decision-edges.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/flow-decision-edges.ts @@ -35,21 +35,23 @@ import { conditionText, type FlowDesignerEdge } from '../previews/flow-canvas-layout.js'; import { uniqueId } from './unique-id.js'; -export interface DecisionEdge { - id?: string; - source: string; - target: string; - /** CEL guard — a bare string or the spec's `{ dialect, source }` envelope. */ - condition?: unknown; - label?: string; - isDefault?: boolean; - type?: string; - [k: string]: unknown; -} +/** + * The edge this module mirrors decision branches onto — the canvas's own edge, + * ALIASED rather than restated (objectui#6287). + * + * It used to be a fourth hand-written copy of that shape whose `condition` was + * still `unknown`, months after objectui#3202 narrowed the designer's edge to + * the spec's `ExpressionInput` — the over-wide read type that describes an + * envelope `FlowEdgeSchema` rejects and that got objectui#3171 filed against a + * defect which does not reproduce. The copy's own `condText` helper existed + * only to cast its way back to the narrow type on every read, which is the + * clearest statement available that the looseness was never wanted here: the + * one value this module ever WRITES to `condition` is a bare CEL string, which + * `ExpressionInput` has always admitted. + */ +export type DecisionEdge = FlowDesignerEdge; -/** `conditionText` narrowed for the loose `unknown` condition this module carries. */ -const condText = (c: unknown): string | undefined => - conditionText(c as FlowDesignerEdge['condition']); +const condText = (c: FlowDesignerEdge['condition']): string | undefined => conditionText(c); /** A branch row as committed by the editor (freeform per the spec config). */ export type DecisionBranch = Record; diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/flow-nested-selection.ts b/packages/app-shell/src/views/metadata-admin/inspectors/flow-nested-selection.ts index 094a18e4d1..9b19f4112a 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/flow-nested-selection.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/flow-nested-selection.ts @@ -102,12 +102,30 @@ export function regionLabelOf(regionKey: string, container?: { config?: unknown // ── C2: node location + write-back ───────────────────────────────────────── -/** A flow node, loose enough for both draft.nodes and a region sub-graph. */ +/** + * A flow node, loose enough for both draft.nodes and a region sub-graph. + * + * This is a READ type over stored metadata, so it stays looser than the spec's + * `FlowNode` where the difference is a real layer difference: `type` and + * `label` are optional here because a node the author has dropped but not + * finished — or a legacy flow read off disk — genuinely occurs without them, + * and a reader that cannot represent what it must open is no safer for being + * strict. The `[k: string]: unknown` index signature is load-bearing for the + * same reason the canvas's `FlowDesignerNode` carries one: node properties this + * layer does not understand are round-tripped rather than dropped. + * + * What it may NOT do is DECLARE a member the contract refuses. `FlowNodeSchema` + * is `.strict()` (objectstack#4001), so a declared `description?: string` said + * an author may write a key that is `unrecognized_keys` in client validation + * and a 422 on save — and the inspector, reading this type, offered a form + * field for it (objectui#6287). `FlowNodeInspector.specKeys.test.tsx` pins the + * declared members (index signature stripped) as a subset of the spec's own + * node keys, so the next addition of that kind fails to compile. + */ export interface FlowNodeLike { id: string; type?: string; label?: string; - description?: string; config?: Record; [k: string]: unknown; }