diff --git a/.changeset/9277-flownode-lockrecord-interrupting-declared-default.md b/.changeset/9277-flownode-lockrecord-interrupting-declared-default.md new file mode 100644 index 0000000000..db8e7dc02a --- /dev/null +++ b/.changeset/9277-flownode-lockrecord-interrupting-declared-default.md @@ -0,0 +1,32 @@ +--- +'@object-ui/app-shell': patch +--- + +Flow-node inspector: `approval.lockRecord` and `boundary_event.boundaryConfig.interrupting` +now declare the `defaultValue` the installed spec applies, so their checkboxes stop +asserting the opposite of what the runtime does (objectui#9277). + +Both keys default to `true` in `@objectstack/spec` (measured on 17.4.0): +`ApprovalNodeConfigSchema` materialises `lockRecord: true` for an approval config that +omits the key, and the `FlowNodeSchema` boundary block materialises +`interrupting: true`. The hand-written descriptor table declared neither. Since +objectui#8451 a boolean config control seeds its checked state from `defaultValue`, so +declaring nothing does not render as "no claim" — it renders as UNCHECKED. An author +opening a node that omits the key read `Lock record` unchecked and believed the record +stayed editable while the request was pending (the runtime locks it), and read +`Interrupting` unchecked and believed the boundary event would leave its host activity +running (it cancels it). + +Both values are derived from the installed spec rather than typed from taste, and the +acceptance pin re-derives them from `ApprovalNodeConfigSchema` / `FlowNodeSchema` at +assertion time instead of restating the literal `'true'`, so the next upstream flip +reddens there rather than drifting silently the way objectui#6620 did. + +Nothing is written. `defaultValue` is shown, never committed (objectui#6263's standing +ruling), and a stored `false` still beats the declaration on both fields — a +deliberately non-locking approval and a non-interrupting boundary event stay authorable +and keep rendering unchecked. + +`patch`, not `minor`: no prop, option or metadata key is added, and no authored document +changes meaning. What changes is that two controls stop contradicting what the runtime +already does with metadata that already parses. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx index eb965a671e..6ccb7f0fc7 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx @@ -84,9 +84,17 @@ import type { MetadataSelection } from '../preview-registry'; // The runtime's own answer for an omitted `escalation` key. Imported so the // #6620 rows below compare the RENDERED control against the installed contract // instead of against a literal this file would then own a second copy of. +// `ApprovalNodeConfigSchema` and `FlowNodeSchema` serve the objectui#9277 rows +// the same way, for `config.lockRecord` and `boundaryConfig.interrupting`, and +// `EndConfigSchema` serves the objectui#9278 rows for `end.outcome`. // ⛔ The subpath is load bearing: `ApprovalEscalationSchema` is NOT on the // package root, where it reads `undefined` and any `.parse` on it throws. -import { ApprovalEscalationSchema, EndConfigSchema, FlowNodeSchema } from '@objectstack/spec/automation'; +import { + ApprovalEscalationSchema, + ApprovalNodeConfigSchema, + EndConfigSchema, + FlowNodeSchema, +} from '@objectstack/spec/automation'; /* ── The `meta/*` double (objectui#7307) ─────────────────────────────── * `FlowNodeInspector` renders `FlowReferenceField` for every reference-kind key @@ -322,8 +330,32 @@ describe('boolean: a declared defaultValue seeds the control (objectui#8451, arm it('boolean: a field declaring NO default still draws unchecked when unset', () => { // The seed belongs to the DECLARATION, not to the control: a boolean with // nothing declared must not acquire a default from the repair. Measured on - // the online writer because the offline table has no undeclared boolean to - // measure — it carries exactly two boolean fields and both declare one. + // the ONLINE writer, whose schema this row spells out inline, so the claim + // rests on a fixture this file owns outright. + // + // ⚠️ The reason that used to stand here — "the offline table has no + // undeclared boolean to measure — it carries exactly two boolean fields + // and both declare one" — was false when written and is still false. + // Swept off `FLOW_NODE_CONFIG` over the same node types the pin below + // sweeps, the offline table carries FIVE boolean fields, not two: + // `approval.escalation.enabled`, `approval.escalation.notifySubmitter`, + // `approval.lockRecord`, `boundary_event.boundaryConfig.interrupting` and + // `screen.waitForInput`. Two of them declared a default when that sentence + // was written (objectui#8586) and four do now, objectui#9277 having + // declared the middle two — so the count it reported was the DECLARING + // booleans mislabelled as the whole boolean surface. + // ⇒ `screen.waitForInput` is the offline undeclared boolean that sentence + // says does not exist — and it is undeclared CORRECTLY, so ⛔ do not read + // this as an objectui#9277-class omission waiting to be declared. Measured + // on the installed `@objectstack/spec` (17.4.0): `waitForInput` is typed + // `z.boolean().optional()` with no `.default(...)`, so an omitted key + // materialises nothing and there is no spec answer for a declaration to + // mirror. Declaring one here would invent a default the runtime does not + // apply — the opposite of what objectui#9277 did for `lockRecord` and + // `interrupting`, where the spec DOES materialise `true`. + // An offline twin of this row is therefore writable today; it is + // deliberately NOT written here, because that is a new assertion rather + // than the merge this file is being changed for. stubs.configSchemas = { approval: { type: 'object', @@ -383,6 +415,123 @@ describe('boolean: a declared defaultValue seeds the control (objectui#8451, arm 'the control carries its label and nothing else — no caption naming a default', ).toBe('SLA escalation'); }); + + /* ── objectui#9277: the other direction of the same ledger, twice ───────── + * `approval.config.lockRecord` and `boundary_event.boundaryConfig.interrupting` + * declared NOTHING while the installed spec materialises `true` for both. + * + * Since objectui#8451 that is not a missing claim — it is a WRONG one: a + * boolean control seeds its checked state from `defaultValue`, so an absent + * declaration draws an UNCHECKED box. The author read `Lock record` unchecked + * and believed the record stayed editable while the node was pending, and + * read `Interrupting` unchecked and believed the boundary event would leave + * its host activity running. The runtime does the opposite of both. + * + * ⭐ Every expectation below is DERIVED from the installed spec, never the + * literal `true`, for the reason the #6620 row above states: a literal + * restates the very claim that drifted and passes just as happily on the next + * upstream flip. Each derivation carries the same vacuity guard — a spec that + * stopped materialising the key would otherwise turn both sides into + * `undefined` and the comparison into a tautology. + * + * Three rows per field, and they are not interchangeable: the DECLARATION row + * pins the table against the contract, the SEEDING row pins the control the + * author actually sees (it reads the spec directly, so it reddens on its own + * when the declaration is dropped), and the STORED-FALSE row is the only one + * an always-checked control — an implementation strictly worse than the bug — + * fails. + * ──────────────────────────────────────────────────────────────── */ + + /** The `lockRecord` the installed spec applies to an approval config that omits it. */ + function specLockRecord(): boolean { + const parsed = ApprovalNodeConfigSchema.safeParse({ approvers: [{ type: 'user', value: 'u1' }] }); + expect(parsed.success, 'a minimal approval config must parse').toBe(true); + const value = (parsed.data as { lockRecord?: unknown } | undefined)?.lockRecord; + expect(typeof value, 'the spec still materialises `lockRecord` from an omitted key').toBe('boolean'); + return value as boolean; + } + + /** The `interrupting` the installed spec applies to a boundaryConfig that omits it. */ + function specInterrupting(): boolean { + const parsed = FlowNodeSchema.safeParse({ + id: 'n1', + type: 'boundary_event', + label: 'Node', + boundaryConfig: { attachedToNodeId: 'host', eventType: 'error' }, + }); + expect(parsed.success, 'a minimal boundary_event node must parse').toBe(true); + const value = (parsed.data as { boundaryConfig?: { interrupting?: unknown } } | undefined) + ?.boundaryConfig?.interrupting; + expect(typeof value, 'the spec still materialises `interrupting` from an omitted key').toBe('boolean'); + return value as boolean; + } + + /** A boundary_event node whose `boundaryConfig` carries only its required keys. */ + const boundaryDraft = (extra: Record = {}) => + draftWith('boundary_event', { boundaryConfig: { attachedToNodeId: 'host', eventType: 'error', ...extra } }); + + it('boolean: approval.lockRecord declares the default the spec applies (objectui#9277)', () => { + const field = fieldsForNodeType('approval').find((f) => f.id === 'lockRecord'); + expect(field?.kind, 'lockRecord is a boolean control').toBe('boolean'); + // Defaults are strings in this table — the spelling `controllerAdmits` compares. + expect( + field?.defaultValue, + 'the table declares the default the spec applies, and it used to declare nothing', + ).toBe(String(specLockRecord())); + }); + + it('boolean: approval.lockRecord draws a CHECKED box for an unset key (objectui#9277)', () => { + // The seeding half, and the ablation target: this row reads the SPEC, not + // the table, so dropping the declaration reddens it here on the rendered + // control rather than only on the row above. + const expected = specLockRecord(); + renderInspector(draftWith('approval', { config: {} })); + const box = checkbox('Lock record'); + expect(box, 'the Lock record control is on screen').not.toBeNull(); + expect( + box!.checked, + 'the RENDERED box carries the state the runtime applies — the defect was that it drew the opposite', + ).toBe(expected); + }); + + it('boolean: a stored `false` still beats lockRecord\'s declaration (objectui#9277)', () => { + // ⛔ Load bearing, and not interchangeable with the row above: an + // ALWAYS-CHECKED control satisfies "an absent key shows checked" and is + // refused only here. + renderInspector(draftWith('approval', { config: { lockRecord: false } })); + expect( + checkbox('Lock record')!.checked, + 'a deliberate false is the author\'s answer and outranks the declaration', + ).toBe(false); + }); + + it('boolean: boundaryConfig.interrupting declares the default the spec applies (objectui#9277)', () => { + const field = fieldsForNodeType('boundary_event').find((f) => f.id === 'boundaryConfig.interrupting'); + expect(field?.kind, 'interrupting is a boolean control').toBe('boolean'); + expect( + field?.defaultValue, + 'the table declares the default the spec applies, and it used to declare nothing', + ).toBe(String(specInterrupting())); + }); + + it('boolean: boundaryConfig.interrupting draws a CHECKED box for an unset key (objectui#9277)', () => { + const expected = specInterrupting(); + renderInspector(boundaryDraft()); + const box = checkbox('Interrupting'); + expect(box, 'the Interrupting control is on screen').not.toBeNull(); + expect( + box!.checked, + 'the RENDERED box says the host activity IS cancelled, which is what the runtime does', + ).toBe(expected); + }); + + it('boolean: a stored `false` still beats interrupting\'s declaration (objectui#9277)', () => { + renderInspector(boundaryDraft({ interrupting: false })); + expect( + checkbox('Interrupting')!.checked, + 'a non-interrupting boundary event is authorable, and the declaration must not overwrite it', + ).toBe(false); + }); }); describe('the online writer of defaultValue reaches the same repaired control', () => { @@ -589,15 +738,23 @@ describe('non-regression — a change that deletes the control must not pass thi describe('the declaration surface this card names', () => { /** - * The eleven declaring fields, as `.`. Triage named + * The thirteen declaring fields, as `.`. Triage named * this list the acceptance surface, so it is pinned: a PR that retires the - * property, or that adds a twelfth declaration, moves this line. + * property, or that adds a fourteenth declaration, moves this line. + * + * ⚠️ The number is NOT a constant to copy, and it is not one card's + * arithmetic either. objectui#9278 and objectui#9277 were both in flight + * against this line from the same base of TEN, each written assuming the + * other does not exist: #9278 landed first and wrote eleven (`end.outcome`, + * select-kind), then #9277 merged it and wrote THIRTEEN after adding two + * boolean-kind declarations (`approval.lockRecord`, + * `boundary_event.boundaryConfig.interrupting`). * - * ⚠️ The number is NOT a constant to copy. objectui#9278 and objectui#9277 - * both move this line from the same base, so each computed it as the value - * standing here when it landed PLUS its own additions, and said in its PR - * body where it read the base from. Do the same rather than trusting either - * card's arithmetic — both were done assuming the other does not exist. + * That thirteen was RE-DERIVED on the merged tree — swept off + * `FLOW_NODE_CONFIG` through `fieldsForNodeType`, exactly as the body below + * does it — and not obtained by adding `11 + 2`. The two happened to agree + * here; that is a result, not a method. Whoever lands next re-reads the + * number the same way rather than trusting this sentence. * * Swept over the picker's node types plus the four that carry config but are * not offered in the picker (ADR-0031 import/export-only, and the legacy @@ -606,7 +763,7 @@ describe('the declaration surface this card names', () => { */ const OFF_PICKER_TYPES = ['boundary_event', 'parallel_gateway', 'join_gateway', 'legacy_action', 'notify']; - it('exactly eleven fields declare a defaultValue, and these are they', () => { + it('exactly thirteen fields declare a defaultValue, and these are they', () => { const swept = [...FLOW_NODE_TYPE_OPTIONS, ...OFF_PICKER_TYPES]; expect( FLOW_NODE_TYPE_OPTIONS.every((t) => swept.includes(t)), @@ -624,9 +781,11 @@ describe('the declaration surface this card names', () => { 'approval.escalation.enabled', 'approval.escalation.notifySubmitter', 'approval.behavior', + 'approval.lockRecord', 'approval.maxRevisions', 'approval.onEmptyApprovers', 'boundary_event.boundaryConfig.eventType', + 'boundary_event.boundaryConfig.interrupting', 'end.outcome', 'http_request.method', 'screen.mode', @@ -668,7 +827,7 @@ describe('the declaration surface this card names', () => { expect( cases.map((c) => c.id).sort(), - 'the select-kind half of the declaration surface — eight of the eleven', + 'the select-kind half of the declaration surface — eight of the thirteen', ).toEqual([ 'approval.behavior', 'approval.escalation.action', diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts index a49413ec42..7fa56dc242 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts @@ -785,7 +785,17 @@ const FLOW_NODE_CONFIG: Record = { placeholder: '1', help: 'Approvals required — total for quorum, per group for per_group. Clamped server-side so it can never deadlock.', }), + // `defaultValue` mirrors the spec's `.default(true)` (objectui#9277): an + // approval config that OMITS `lockRecord` parses as LOCKED, so a table + // declaring nothing drew the box UNCHECKED — the inspector told the author + // the record stayed editable while the node was pending, and the runtime + // locked it. Since objectui#8451 seeded the boolean control from this + // property, an absent declaration is not a missing claim here; it is the + // wrong one. Derived from the installed `ApprovalNodeConfigSchema`, never + // from taste — `FlowNodeInspector.declaredDefault.test.tsx` re-derives it + // from that schema rather than restating the literal. cfg('lockRecord', 'Lock record', 'boolean', { + defaultValue: 'true', help: 'Lock the triggering record from edits while this node is pending.', }), cfg('approvalStatusField', 'Status field', 'reference', { @@ -946,7 +956,16 @@ const FLOW_NODE_CONFIG: Record = { ], defaultValue: 'error', }), - at('boundaryConfig', 'interrupting', 'Interrupting', 'boolean', { help: 'Cancel the host activity when this event fires.' }), + // `defaultValue` mirrors the spec's `.default(true)` (objectui#9277): a + // `boundaryConfig` that OMITS `interrupting` parses as INTERRUPTING, so a + // table declaring nothing drew the box UNCHECKED and told the author the + // host activity would keep running — it is cancelled. Same shape as + // `lockRecord` above, derived from the installed `FlowNodeSchema` boundary + // block and re-derived in `FlowNodeInspector.declaredDefault.test.tsx`. + at('boundaryConfig', 'interrupting', 'Interrupting', 'boolean', { + defaultValue: 'true', + help: 'Cancel the host activity when this event fires.', + }), at('boundaryConfig', 'errorCode', 'Error code', 'text', { placeholder: 'TIMEOUT (empty = all)', showWhen: { field: 'boundaryConfig.eventType', equals: ['error'] },