From f5c604ee1a8c23c12c8d503534d55d9316f0781f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 00:51:05 +0000 Subject: [PATCH] fix(app-shell): seed the flow-node boolean control from the declared defaultValue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit objectui#6830 arm A ("show, do not write", triage 2026-09-04), the boolean half — objectui#8451. The select half stays untouched: it is blocked on objectui#8450. The control drew `value === true`, which flattened an ABSENT key and a stored `false` onto one unchecked box although the runtime treats them oppositely on a key the spec defaults to true. It now shows the value in effect — the stored boolean, else the declared `defaultValue` in the same 'true'/'false' spelling `controllerAdmits` already resolves an unset controller against. Nothing is written; the first author edit still commits an explicit boolean. `isUnsetFieldValue` is exported from `flow-node-config.ts` so the three sites that have to answer "has the author stored anything here" (the re-show rule, the `showWhen` resolver, and now the control) share one definition instead of three copies of `=== undefined || === null || === ''`. ⛔ No caption naming the default is rendered, deliberately. Of the two boolean fields the offline table declares a default for, `escalation.enabled` declares the OPPOSITE of what installed `@objectstack/spec` 17.3.0 applies to the omitted key (objectui#6620), so a "(default)" label would have shipped that wrong claim to authors in words. The seed alone leaves that field rendering byte-identically to before, which is pinned. PR #8431's pin is turned green against the repaired behaviour rather than weakened: its three leg-B rows now assert the new states, and the stored-`false` rows are kept load bearing so an always-checked control — an implementation strictly worse than the bug — still fails. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S --- .../8451-flownode-boolean-declared-default.md | 32 +++ .../inspectors/FlowNodeConfigField.tsx | 37 ++- ...FlowNodeInspector.declaredDefault.test.tsx | 212 ++++++++++++++---- .../inspectors/flow-node-config.ts | 33 ++- 4 files changed, 269 insertions(+), 45 deletions(-) create mode 100644 .changeset/8451-flownode-boolean-declared-default.md diff --git a/.changeset/8451-flownode-boolean-declared-default.md b/.changeset/8451-flownode-boolean-declared-default.md new file mode 100644 index 0000000000..9f000ea0dd --- /dev/null +++ b/.changeset/8451-flownode-boolean-declared-default.md @@ -0,0 +1,32 @@ +--- +'@object-ui/app-shell': patch +--- + +Flow-node inspector: a boolean config control now shows the declared default when +the node omits the key (objectui#8451, objectui#6830 arm A "show, do not write"). + +Before this, the boolean control drew `value === true`, which flattened an ABSENT +key and a stored `false` onto the same unchecked box even though the runtime treats +them oppositely on a key the spec defaults to `true`. On an approval node that omits +`escalation.notifySubmitter`, the inspector showed "Notify submitter" unticked while +the executor notified the submitter — a rendered false statement about what the flow +will do, not a missing hint. + +The control now reads the value in effect: the stored boolean, or — when nothing is +stored — the `defaultValue` the descriptor declares, in the same `'true'`/`'false'` +spelling `isFieldVisible` already resolves an unset `showWhen` controller against. +Both writers of the property feed it: the hand-written descriptor table and the +engine-published `configSchema` that `json-schema-to-fields` converts. + +**Nothing is written.** A node that never had the key still ships without it; the +first author edit commits an explicit boolean exactly as before. Ticking a box that +shows a declared `true` writes `false`, which is the author's answer and outranks +the declaration on every later render. + +`patch`, not `minor`: no prop, option or metadata key is added, and no authored +document changes meaning. What changes is that one control stops contradicting the +runtime for metadata that already parses. + +Scope is the boolean control only. The select control is unchanged — rendering an +effective default there needs `InspectorSelectField`'s unreachable `placeholder` +prop fixed first (objectui#8450). diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx index b8114aaf2a..0851038685 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx @@ -7,6 +7,7 @@ */ import * as React from 'react'; +import { isUnsetFieldValue } from './flow-node-config.js'; import type { FlowConfigField, InactiveRetainedKind } from './flow-node-config.js'; import { t } from '../i18n.js'; import { @@ -199,15 +200,47 @@ export function FlowNodeConfigField({ field, value, onCommit, disabled, locale, disabled={disabled} /> ); - case 'boolean': + case 'boolean': { + // objectui#6830 arm A — "show, do not write" (triage 2026-09-04), the + // boolean half (objectui#8451). + // + // The control shows the value IN EFFECT: the stored boolean, or — when + // the author has stored nothing — the `defaultValue` the descriptor + // declares, which is what the runtime applies to the omitted key. + // Nothing is WRITTEN: an untouched node still ships without the key, + // and the first author edit commits an explicit boolean as before. + // + // The seed used to be `value === true`, which flattened `undefined` and + // `false` onto one unchecked box although the runtime treats them + // oppositely on a `default(true)` key — the mechanism that turned a + // missing display into a false assertion. The distinction survives to + // here (`getFieldValue` returns `undefined` vs `false`), so the two + // branches are kept apart: + // + // - unset -> the declared default in the table's own 'true'/'false' + // spelling, the one `controllerAdmits` compares a controller against; + // - stored -> `=== true`, unchanged. Deliberately NOT widened to also + // accept a stored string `'true'`: that would be a lenient + // renderer-side alias for off-spec metadata (AGENTS.md #0.1). + // + // ⛔ Nothing is rendered NAMING the default — no "(default)" caption, + // no help line. Of the two boolean fields the offline table declares a + // default for, one (`escalation.enabled`) declares the OPPOSITE of what + // the installed spec applies to an omitted key (objectui#6620), so any + // caption asserting "this is the declared default" would ship that + // wrong claim to authors in words. The seed alone leaves that field + // rendering byte-identically to before; a caption would not. Pinned by + // `FlowNodeInspector.declaredDefault.test.tsx`'s #6620 case. + const checked = isUnsetFieldValue(value) ? field.defaultValue === 'true' : value === true; return ( onCommit(v)} disabled={disabled} /> ); + } case 'select': return (() => { const current = value != null ? String(value) : ''; 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 0dd3dae7f2..ca747dc66b 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 @@ -12,32 +12,47 @@ * declaration table would be a source-text snapshot and would pass on a build * where nothing rendered at all. * + * ⚑ objectui#8451 rewrote the BOOLEAN half of this file. Triage ruled arm A + * ("show, do not write") on objectui#6830, and the boolean control now seeds + * from the declared default, so the three rows that pinned the defect were + * turned green against the repaired behaviour rather than deleted — the same + * three PR #8431's ablation leg B predicted would move. Everything else is + * carried over unchanged, including every select case: that half is blocked on + * objectui#8450 and is still unrepaired. + * * The measurement, in four parts: * - * 1. A declared default does NOT seed the control's value. An unset key draws - * an unchecked box / a blank select, whatever the table declares. The - * select is blank rather than showing a placeholder, because the field - * always passes a controlled value and Radix renders its placeholder only - * for an undefined one. - * 2. That holds on BOTH writers of the property — the hand-written table here - * and the engine-published `configSchema` that `json-schema-to-fields` - * converts (`default: true` -> `defaultValue: 'true'`). Same dead end. - * 3. It is NOT inert, though: a declared default on a `showWhen` CONTROLLER - * changes which fields are on screen at all (`controllerAdmits`). That is - * the one read site, and it is a visibility effect, not a value effect. - * 4. A boolean control cannot distinguish "key absent" from "key stored as - * `false`" — the mechanism that turns a missing display into a false - * assertion. + * 1. On a BOOLEAN control a declared default now seeds the value: an unset key + * draws the declared state, a stored value beats it, and nothing is written + * to the node. On a SELECT it still reaches nothing — an unset key draws a + * blank trigger, not the declared option and not even a placeholder, + * because the field always passes a controlled value and Radix renders its + * placeholder only for an undefined one (objectui#8450). + * 2. Both writers of the property feed the repaired boolean — the hand-written + * table here and the engine-published `configSchema` that + * `json-schema-to-fields` converts (`default: true` -> `defaultValue: + * 'true'`). The select is a dead end on both. + * 3. The property also drives VISIBILITY: a declared default on a `showWhen` + * CONTROLLER changes which fields are on screen at all (`controllerAdmits`). + * That read site predates the repair and is unchanged by it. + * 4. A boolean control can now distinguish "key absent" from "key stored as + * `false`" — the two used to render byte-identical DOM even though the + * runtime treats them oppositely, which is the mechanism that turned a + * missing display into a false assertion. + * + * Two describes guard the repair from below: * - * The last describe is the non-regression half (objectui#8350's lesson): every - * negative above is also satisfied by an inspector that renders NOTHING, so the - * controls' existence and their stored-value behaviour are pinned beside them. - * A change that deletes the boolean branch fails here even though it would - * satisfy "the control shows no default". + * - the non-regression half (objectui#8350's lesson): every claim here is also + * satisfied by an inspector that renders NOTHING, so the controls' + * existence, their stored-value behaviour, the offered vocabulary and the + * deprecated-value fallback are pinned beside them. + * - the stored-`false` rows: an implementation strictly WORSE than the bug — + * a box that is ALWAYS checked — satisfies "an absent key shows checked". + * Only a stored `false` refuses it, so those rows are load bearing and must + * not be softened into "differs from absent". * - * ⛔ These cases pin what the tree DOES today. They are not a ruling that it is - * right — the direction (show the effective default vs. retire the property) is - * a product call recorded on the card. + * ⛔ These cases pin what the tree DOES. The direction they pin is triage's + * ruling on objectui#6830, not this file's opinion. */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; @@ -143,7 +158,7 @@ const triggerText = (name: string) => screen.getByRole('combobox', { name }).tex const checkbox = (name: string) => screen.queryByLabelText(name) as HTMLInputElement | null; -describe('a declared defaultValue does not reach the rendered control', () => { +describe('select: a declared defaultValue still does not reach the control (objectui#8450)', () => { it('select: an unset key draws the placeholder, not the declared "GET"', () => { // Premise, read from the table the inspector renders from. const method = fieldsForNodeType('http_request').find((f) => f.id === 'method'); @@ -174,8 +189,10 @@ describe('a declared defaultValue does not reach the rendered control', () => { ).toBe('GET'); expect(screen.queryByText('GET'), 'the lit control fires').not.toBeNull(); }); +}); - it('boolean: an unset key draws an UNCHECKED box, though the table declares true', () => { +describe('boolean: a declared defaultValue seeds the control (objectui#8451, arm A)', () => { + it('boolean: an unset key draws a CHECKED box, because the table declares true', () => { const notify = fieldsForNodeType('approval').find((f) => f.id === 'escalation.notifySubmitter'); expect(notify?.defaultValue, 'escalation.notifySubmitter declares a default').toBe('true'); @@ -187,16 +204,98 @@ describe('a declared defaultValue does not reach the rendered control', () => { expect(box, 'the Notify submitter control is rendered').not.toBeNull(); expect( box!.checked, - 'the box reads UNCHECKED while the runtime treats the omitted key as true', - ).toBe(false); + 'the box reads CHECKED, which is what the runtime applies to the omitted key', + ).toBe(true); + }); + + it('boolean: showing the default WRITES nothing — the node still omits the key', () => { + // The other half of "show, do not write". A seeded control that also + // committed would turn every visit to the inspector into a metadata edit, + // freezing today's default into the node and un-tracking it from the spec. + const { onPatch } = renderInspector( + draftWith('approval', { config: { escalation: { enabled: true, timeoutHours: 24 } } }), + ); + expect(checkbox('Notify submitter')!.checked, 'the seed is on screen').toBe(true); + expect( + onPatch.mock.calls, + 'rendering a seeded control patches the draft exactly zero times', + ).toEqual([]); }); - it('boolean: the same box IS checked when the key is stored true — the lit control', () => { + it('boolean: a stored true renders checked — unchanged by the repair', () => { renderInspector( draftWith('approval', { config: { escalation: { enabled: true, timeoutHours: 24, notifySubmitter: true } } }), ); expect(checkbox('Notify submitter')!.checked, 'a stored true renders checked').toBe(true); }); + + it('boolean: a stored FALSE beats the declared true and draws an UNCHECKED box', () => { + // ⛔ Load bearing, and not interchangeable with "differs from the absent + // rendering": this is the only row an ALWAYS-CHECKED control fails. Without + // it, an implementation strictly worse than the bug — one that ignores both + // the stored value and the declaration — satisfies every other claim here. + renderInspector( + draftWith('approval', { config: { escalation: { enabled: true, timeoutHours: 24, notifySubmitter: false } } }), + ); + expect( + checkbox('Notify submitter')!.checked, + 'a deliberate false is the author\'s answer and outranks the declaration', + ).toBe(false); + }); + + 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. + stubs.configSchemas = { + approval: { + type: 'object', + properties: { + escalation: { + type: 'object', + title: 'SLA escalation', + properties: { + enabled: { type: 'boolean', default: true }, + notifySubmitter: { type: 'boolean', title: 'Notify submitter' }, + }, + }, + }, + }, + }; + renderInspector(draftWith('approval', { config: { escalation: { timeoutHours: 24 } } })); + const box = checkbox('Notify submitter'); + expect(box, 'the sibling is on screen — the gate default admitted it').not.toBeNull(); + expect( + box!.checked, + 'and it draws unchecked, because this schema declares no default for it', + ).toBe(false); + }); + + it('boolean: the #6620-wrong declaration ships NO worded claim (objectui#6620)', () => { + // `escalation.enabled` is the one field in the offline table whose declared + // default contradicts what the installed spec applies to an omitted key. + // This card ships the SEED and no caption, which is why: the seed renders + // that field exactly as it rendered before (unchecked — the declaration + // says 'false'), so no new claim about the declaration reaches the author, + // while a "(default)" caption would have asserted the wrong one in words. + // + // ⚠️ Deliberately does NOT compare the declaration against the spec. That + // comparison is objectui#6620's tripwire, held disarmed on purpose in + // `flow-node-config.spec-reconciliation.test.ts`; arming it here would + // discharge an on-hold card from an unrelated PR. + const gate = fieldsForNodeType('approval').find((f) => f.id === 'escalation.enabled'); + expect(gate?.defaultValue, 'the gate declares a default at all').toBe('false'); + + renderInspector(draftWith('approval', { config: { escalation: { timeoutHours: 24 } } })); + const box = checkbox('SLA escalation'); + expect(box, 'the gate control is on screen').not.toBeNull(); + expect(box!.checked, 'and it renders as it always did — unchecked').toBe(false); + expect( + box!.closest('label')?.textContent, + 'the control carries its label and nothing else — no caption naming a default', + ).toBe('SLA escalation'); + }); }); describe('the online writer of defaultValue hits the same dead end', () => { @@ -248,10 +347,17 @@ describe('the one read site: a declared default on a showWhen CONTROLLER changes it('a gate whose default is true reveals its siblings for a node that omits the key', () => { stubs.configSchemas = { approval: escalationSchema }; renderInspector(draftWith('approval', { config: { escalation: { timeoutHours: 24 } } })); + const sibling = checkbox('Notify submitter'); expect( - checkbox('Notify submitter'), + sibling, 'the sibling is on screen because the gate default resolved to true', ).not.toBeNull(); + // objectui#8451 — and the ONLINE writer's own `default: true` seeds it, so + // the repair is not a property of the hand-written table. + expect( + sibling!.checked, + 'the engine-published default reaches the control too', + ).toBe(true); }); it('and hides them when the gate is stored off — the lit control for the same predicate', () => { @@ -275,24 +381,45 @@ describe('the one read site: a declared default on a showWhen CONTROLLER changes }); }); -describe('absent and stored-false are indistinguishable on a boolean control', () => { - it('renders identically whether the key is missing or explicitly false', () => { +describe('absent and stored-false are now DISTINGUISHABLE on a boolean control', () => { + it('renders differently when the key is missing than when it is explicitly false', () => { renderInspector(draftWith('approval', { config: { escalation: { enabled: true, timeoutHours: 24 } } })); - const absent = checkbox('Notify submitter')!.outerHTML; + const absentBox = checkbox('Notify submitter')!; + const absentChecked = absentBox.checked; + const absent = absentBox.outerHTML; cleanup(); renderInspector( draftWith('approval', { config: { escalation: { enabled: true, timeoutHours: 24, notifySubmitter: false } } }), ); - const storedFalse = checkbox('Notify submitter')!.outerHTML; + const storedFalseBox = checkbox('Notify submitter')!; + const storedFalseChecked = storedFalseBox.checked; + const storedFalse = storedFalseBox.outerHTML; + + // Named states, not just an inequality: "the two differ" is also satisfied + // by a control that gets BOTH wrong, so each side is asserted on its own. + expect(absentChecked, 'the omitted key draws the declared true').toBe(true); + expect(storedFalseChecked, 'the deliberate false draws unchecked').toBe(false); expect( absent, - 'an omitted key and a deliberate false draw the same control — the author cannot tell them apart', - ).toBe(storedFalse); + 'and the difference reaches the DOM — the author can tell them apart', + ).not.toBe(storedFalse); }); }); describe('non-regression — a change that deletes the control must not pass this file', () => { + /** The `notifySubmitter` value the last `onPatch` call carries, if any. */ + const committed = (onPatch: { mock: { calls: unknown[][] } }) => + ( + onPatch.mock.calls.at(-1)?.[0] as + | { nodes: Array<{ config: { escalation?: Record } }> } + | undefined + )?.nodes[0].config.escalation?.notifySubmitter; + it('the boolean control exists, is a checkbox, and commits the author edit', () => { + // Both directions, because the repair moved the seeded state: clicking a + // control that shows the declared `true` must write the author's `false`, + // and clicking one that shows a stored `false` must write `true`. A control + // that committed the state it merely SHOWS would pass one and fail the other. const { onPatch } = renderInspector( draftWith('approval', { config: { escalation: { enabled: true, timeoutHours: 24 } } }), ); @@ -300,12 +427,19 @@ describe('non-regression — a change that deletes the control must not pass thi expect(box, 'the control is rendered at all').not.toBeNull(); expect(box!.type, 'and it is a real checkbox input').toBe('checkbox'); box!.click(); - const patched = onPatch.mock.calls.at(-1)?.[0] as - | { nodes: Array<{ config: { escalation?: Record } }> } - | undefined; expect( - patched?.nodes[0].config.escalation?.notifySubmitter, - 'ticking the box writes the key — the control is live, not decorative', + committed(onPatch), + 'clicking a seeded-true box writes the explicit false — the control is live, not decorative', + ).toBe(false); + + cleanup(); + const second = renderInspector( + draftWith('approval', { config: { escalation: { enabled: true, timeoutHours: 24, notifySubmitter: false } } }), + ); + checkbox('Notify submitter')!.click(); + expect( + committed(second.onPatch), + 'and clicking a stored-false box writes true', ).toBe(true); }); 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 8dd4fd3856..e89357d701 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 @@ -225,7 +225,15 @@ export interface FlowConfigField { options?: Array<{ value: string; label: string }>; /** One-line helper hint shown under the control. */ help?: string; - /** Spec default, used when resolving `showWhen` against an unset controller. */ + /** + * The spec default for this key, in the `'true'`/`'false'` string spelling + * this table compares against. Read at TWO sites, both for an unset key: + * `isFieldVisible` resolves a `showWhen` controller through it (a declared + * default decides which fields are on screen), and — since objectui#8451, + * objectui#6830 arm A — a `boolean` control seeds its checked state from + * it, so the box shows the value the runtime applies rather than a blank + * `false`. Shown, never written: it does not become part of the node. + */ defaultValue?: string; /** * Conditional visibility: only render this field when the controlling field @@ -1098,6 +1106,24 @@ export function isFieldVisible( return controllerAdmits(field, node, fields); } +/** + * Whether a read value counts as UNSET — the one definition of "the author has + * stored nothing here", shared by every site that has to answer it. + * + * There were three copies of this predicate before objectui#8451: the re-show + * rule's {@link hasStoredValue}, {@link controllerAdmits}'s fallback to the + * declared default, and (as of that card) the boolean control's own fallback in + * `FlowNodeConfigField`. The third is the reason it is exported and named: a + * control that seeds from `defaultValue` must call a key unset in exactly the + * words the visibility resolver does, or the same node can render a field whose + * gate says "unset, so use the default" beside a control that says "stored, so + * ignore it". `''` is included because a cleared text control commits the empty + * string rather than deleting the key. + */ +export function isUnsetFieldValue(raw: unknown): boolean { + return raw === undefined || raw === null || raw === ''; +} + /** * Whether this field currently holds a stored value — the input to the * re-show rule above, named so {@link inactiveRetainedKind} asks the same @@ -1107,8 +1133,7 @@ function hasStoredValue( field: FlowConfigField, node: Record | null | undefined, ): boolean { - const own = getFieldValue(node, field); - return own !== undefined && own !== null && own !== ''; + return !isUnsetFieldValue(getFieldValue(node, field)); } /** @@ -1130,7 +1155,7 @@ function controllerAdmits( const controller = fields.find((f) => f.id === field.showWhen!.field); if (!controller) return false; const raw = getFieldValue(node, controller); - const resolved = raw === undefined || raw === null || raw === '' ? controller.defaultValue : raw; + const resolved = isUnsetFieldValue(raw) ? controller.defaultValue : raw; // Boolean controllers (e.g. `escalation.enabled`) compare against 'true'/'false'. const value = typeof resolved === 'boolean' ? String(resolved) : resolved; return typeof value === 'string' && field.showWhen.equals.includes(value);