From 930c7a72584b92dd0df5c3bbd84ef4dca39ba236 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 18:22:39 +0000 Subject: [PATCH 1/4] fix(app-shell): switching an approval node's SLA escalation off removes the escalation block (objectui#11660) The flow inspector wrote `escalation: { enabled: false }` when an author turned escalation off. `ApprovalNodeConfigSchema` refuses that block for its missing `timeoutHours`, so the flow saved and then failed at the approval node. The spec names the block's existence as the switch, so off is now no block: - committing `false` to `config.escalation.enabled` removes `config.escalation` (siblings untouched, an emptied `config` pruned as before); - the switch reads an absent block as off instead of applying its declared default with nothing to apply it to; - a write that would leave the bare `{ enabled: false }` stub removes it. The rule is keyed by node type and path (`BLOCK_SWITCHES`), so it holds for the offline field table and the engine-published configSchema alike. Claude-Session: https://claude.ai/code/session_015W8GBu6sBiqus2L2xjMsAL Co-authored-by: Claude --- .../11660-escalation-off-removes-block.md | 15 + ...NodeInspector.escalationOff-11660.test.tsx | 337 ++++++++++++++++++ ...lowNodeInspector.inactiveRetained.test.tsx | 8 +- .../inspectors/FlowNodeInspector.tsx | 24 +- .../inspectors/flow-node-config.ts | 112 +++++- 5 files changed, 491 insertions(+), 5 deletions(-) create mode 100644 .changeset/11660-escalation-off-removes-block.md create mode 100644 packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.escalationOff-11660.test.tsx diff --git a/.changeset/11660-escalation-off-removes-block.md b/.changeset/11660-escalation-off-removes-block.md new file mode 100644 index 0000000000..e019bbe876 --- /dev/null +++ b/.changeset/11660-escalation-off-removes-block.md @@ -0,0 +1,15 @@ +--- +'@object-ui/app-shell': patch +--- + +Switching an approval node's SLA escalation off in the flow designer removes the node's `escalation` block (objectui#11660). + +The inspector used to write `escalation: { enabled: false }` when an author turned the switch off. `ApprovalNodeConfigSchema` refuses that block, because `timeoutHours` is required whether or not `enabled` is `false`. So the flow saved and then failed at the approval node on every run, and once the platform checks node config at save time, the save itself is refused. The spec states what the switch means: "the feature-level switch is whether the escalation block exists at all". So off is now no block: + +- **Switching off removes the block**, with every value stored in it. The node's other config keys are not touched. A `config` left empty is dropped, as it is for any other cleared field. +- **No block reads off.** A node without an `escalation` block used to draw the switch on, with its four fields revealed, because the declared default (`true`) was applied with nothing to apply it to. It now draws the switch off. A block that omits `enabled` still reads on, and a stored `enabled: false` still reads off. +- **No bare `{ enabled: false }` block is written.** Clearing the last value kept under an older switched-off block removes the block too. A block that still holds another value is kept as it is. + +Both field sources behave the same: the offline field table and the `configSchema` the engine publishes for the approval node. A stored `{ enabled: false, timeoutHours: 24 }` from older metadata still reads off, shows its value as kept but not in effect, and is not rewritten when the node is opened. + +**Clause-②: no.** Nothing on the package entry changes. The new helpers (`switchedBlockOf`, `readFieldValue`, `isBareSwitchedOffBlock`) live in the inspector's own module, which `@object-ui/app-shell` does not export. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.escalationOff-11660.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.escalationOff-11660.test.tsx new file mode 100644 index 0000000000..d8451970b9 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.escalationOff-11660.test.tsx @@ -0,0 +1,337 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#11660 — switching an approval node's SLA escalation OFF removes the + * `escalation` block; it never writes the `{ enabled: false }` stub. + * + * The governing text is the spec's own: `ApprovalEscalationSchema.enabled` is + * described as "the feature-level switch is whether the escalation block exists + * at all", and the block's `timeoutHours` is required whatever `enabled` says. + * Triage ruled the direction (comment 6000059097 on the card): turning + * escalation off removes the block — no `enabled: false` stub, no + * `timeoutHours` kept behind a disabled toggle — and the reading follows from + * it: no block reads OFF, a block that omits `enabled` reads ON (the + * objectui#6620 pin), a stored `enabled: false` reads OFF. + * + * Every case runs twice, once per DESCRIPTOR SOURCE, because the inspector + * renders `serverFields ?? fieldsForNodeType(...)`: + * + * - `offline` — the hand-written table in `flow-node-config.ts`; + * - `engine` — `getApprovalNodeConfigJsonSchema()` from the installed + * `@objectstack/spec/automation`, fed as the approval node's published + * `configSchema`. That function is what objectstack's `plugin-approvals` + * hands its approval node descriptor as `configSchema` (`approval-node.ts`, + * read at objectstack `6afb1b55`), so this is the shape a live backend + * publishes, not a hand-written imitation of it. It labels the fields from + * the schema (`Escalation`, `Timeout Hours`, …), which is why the labels + * below are per source. + * + * Round-trips are judged by `ApprovalNodeConfigSchema` imported from the + * `@objectstack/spec/automation` SUBPATH — `ApprovalEscalationSchema` is not on + * the package root. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, cleanup, fireEvent, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +const stubs = vi.hoisted(() => ({ configSchemas: {} as Record })); + +vi.mock('../previews/useFlowNodePalette', () => ({ + useActionConfigSchemas: () => stubs.configSchemas, + useFlowNodePalette: () => [], +})); +vi.mock('../previews/useObjectFields', () => ({ + useObjectFields: () => ({ fields: [], loading: false, error: null }), +})); + +import { FlowNodeInspector } from './FlowNodeInspector'; +import type { MetadataSelection } from '../preview-registry'; +import { ApprovalNodeConfigSchema, getApprovalNodeConfigJsonSchema } from '@objectstack/spec/automation'; + +/* ── The `meta/*` double (objectui#7307), as the sibling inspector files + * serve it: an empty registry in the `{ type, items: [] }` envelope, and an + * `afterEach` that fails on any URL outside the metadata routes. ── */ +const META_PREFIX = '/api/v1/meta/'; +let calls: string[] = []; +const routeOf = (url: string) => url.split('?')[0]; + +beforeEach(() => { + stubs.configSchemas = {}; + calls = []; + vi.stubGlobal( + 'fetch', + vi.fn(async (input: unknown) => { + const url = String( + input && typeof input === 'object' && 'url' in input ? (input as { url: unknown }).url : input, + ); + calls.push(url); + const route = routeOf(url); + if (!route.startsWith(META_PREFIX)) { + return { ok: false, status: 404, headers: new Headers(), json: async () => ({}) }; + } + return { + ok: true, + status: 200, + headers: new Headers(), + json: async () => ({ type: route.slice(META_PREFIX.length), items: [] }), + }; + }), + ); +}); + +afterEach(() => { + expect(calls.filter((url) => !routeOf(url).startsWith(META_PREFIX))).toEqual([]); + cleanup(); + vi.unstubAllGlobals(); +}); + +type Config = Record; +type Draft = { nodes: Array>; edges: unknown[] }; + +const APPROVERS = [{ type: 'user', value: 'usr_approver' }]; + +function draftWith(config: Config): Draft { + return { nodes: [{ id: 'gate', type: 'approval', label: 'Gate', config }], edges: [] }; +} + +const configOf = (draft: Draft): Config | undefined => draft.nodes[0].config as Config | undefined; + +/** + * The editor loop: every patch is applied to the draft the inspector renders + * next, as the host does. `latest()` is the draft a save would send. + */ +function mount(initial: Draft) { + let current = initial; + const patches: unknown[] = []; + function Host() { + const [draft, setDraft] = React.useState(initial); + return ( + { + patches.push(patch); + setDraft((d) => { + current = { ...d, ...(patch as Partial) }; + return current; + }); + }} + onClearSelection={vi.fn()} + readOnly={false} + locale="en-US" + /> + ); + } + const utils = render(); + return { ...utils, patches, latest: () => current }; +} + +interface Labels { + gate: string; + timeout: string; + action: string; + escalateTo: string; + notify: string; + reassign: string; +} + +const SOURCES: Array<{ name: 'offline' | 'engine'; labels: Labels; install: () => void }> = [ + { + name: 'offline', + labels: { + gate: 'SLA escalation', + timeout: 'Timeout (hours)', + action: 'On timeout', + escalateTo: 'Escalate to', + notify: 'Notify submitter', + reassign: 'Reassign', + }, + install: () => { + stubs.configSchemas = {}; + }, + }, + { + name: 'engine', + labels: { + gate: 'Escalation', + timeout: 'Timeout Hours', + action: 'Action', + escalateTo: 'Escalate To', + notify: 'Notify Submitter', + reassign: 'Reassign', + }, + install: () => { + stubs.configSchemas = { approval: getApprovalNodeConfigJsonSchema() }; + }, + }, +]; + +const gateBox = (l: Labels) => screen.queryByLabelText(l.gate) as HTMLInputElement | null; +const subLabels = (l: Labels) => [l.timeout, l.action, l.escalateTo, l.notify]; +const shownSubFields = (l: Labels) => subLabels(l).filter((label) => screen.queryByText(label) !== null); +const retainedNotices = () => screen.queryAllByTestId('inactive-retained'); + +/** The free-text input of the `Escalate to` reference field (no data adapter is mounted, so it is a plain input). */ +function escalateToInput(l: Labels): HTMLInputElement { + const label = screen.getByText(l.escalateTo); + const input = label.parentElement?.querySelector('input'); + if (!input) throw new Error(`no input beside '${l.escalateTo}'`); + return input as HTMLInputElement; +} + +for (const source of SOURCES) { + const l = source.labels; + + describe(`${source.name} descriptors: a node with no escalation block reads OFF (objectui#11660)`, () => { + beforeEach(source.install); + + it('the toggle draws unchecked and no escalation field is revealed', () => { + const draft = draftWith({ approvers: APPROVERS }); + const before = JSON.stringify(draft); + const { patches } = mount(draft); + const box = gateBox(l); + expect(box, 'the switch is on screen').not.toBeNull(); + expect(box!.checked, 'no block is OFF — the spec calls the block itself the switch').toBe(false); + expect(shownSubFields(l), 'none of the four escalation fields is revealed over no block').toEqual([]); + // A read: rendering the corrected toggle writes nothing. + expect(patches, 'rendering patches the draft zero times').toEqual([]); + expect(JSON.stringify(draft), 'and the draft is unchanged byte for byte').toBe(before); + }); + + it('lit control: a block that omits `enabled` still reads ON and reveals its fields (objectui#6620)', () => { + mount(draftWith({ approvers: APPROVERS, escalation: { timeoutHours: 24 } })); + expect(gateBox(l)!.checked, 'inside a block, the spec default `true` applies').toBe(true); + expect(shownSubFields(l)).toEqual(subLabels(l)); + }); + + it('a stored `enabled: false` reads OFF', () => { + mount(draftWith({ approvers: APPROVERS, escalation: { enabled: false } })); + expect(gateBox(l)!.checked).toBe(false); + expect(shownSubFields(l)).toEqual([]); + }); + }); + + describe(`${source.name} descriptors: switching escalation off removes the block (objectui#11660)`, () => { + beforeEach(source.install); + + it('a block holding every value leaves no `escalation` key, and every sibling config key byte-identical', () => { + const siblings: Config = { + approvers: APPROVERS, + behavior: 'quorum', + minApprovals: 1, + onEmptyApprovers: 'fail', + lockRecord: false, + maxRevisions: 2, + // An Advanced (JSON) extra — a key no form field owns. + custom_note: { kept: ['as', 'is'] }, + }; + const config: Config = { + ...siblings, + escalation: { enabled: true, timeoutHours: 24, action: 'reassign', escalateTo: 'sre_lead', notifySubmitter: false }, + }; + const { latest } = mount(draftWith(config)); + expect(gateBox(l)!.checked, 'premise: escalation is on').toBe(true); + + gateBox(l)!.click(); + + const after = configOf(latest())!; + expect('escalation' in after, 'no `escalation` key is left behind — not a stub, not a hidden block').toBe(false); + // Byte-identical siblings, key order included: the removal deletes one key + // and rebuilds nothing else. + expect(JSON.stringify(after)).toBe(JSON.stringify(siblings)); + + // What the author sees next: the switch off, nothing revealed, and nothing + // flagged as retained — there is nothing left to retain. + expect(gateBox(l)!.checked, 'the toggle reads the absent block as off').toBe(false); + expect(shownSubFields(l)).toEqual([]); + expect(retainedNotices()).toHaveLength(0); + }); + + it('a block that omits `enabled` (switch drawn on by default) is removed too', () => { + const { latest } = mount(draftWith({ approvers: APPROVERS, escalation: { timeoutHours: 8 } })); + gateBox(l)!.click(); + expect(configOf(latest())).toEqual({ approvers: APPROVERS }); + }); + + it('an emptied `config` is pruned with the block', () => { + const { latest } = mount(draftWith({ escalation: { enabled: true, timeoutHours: 24 } })); + gateBox(l)!.click(); + expect(latest().nodes[0], 'removing the only config key leaves no empty `config` object').not.toHaveProperty('config'); + }); + }); + + describe(`${source.name} descriptors: an inspector-authored node round-trips through ApprovalNodeConfigSchema (objectui#11660)`, () => { + beforeEach(source.install); + + it('escalation ON with every value parses and keeps every value; OFF leaves no block and parses', async () => { + const { latest } = mount(draftWith({ approvers: APPROVERS })); + expect(gateBox(l)!.checked, 'premise: a fresh node starts off').toBe(false); + + // ON, authored control by control through the inspector. + gateBox(l)!.click(); + expect(gateBox(l)!.checked, 'switching on draws the switch on').toBe(true); + fireEvent.change(screen.getByLabelText(l.timeout), { target: { value: '24' } }); + await userEvent.click(screen.getByRole('combobox', { name: l.action })); + await userEvent.click(await screen.findByRole('option', { name: l.reassign })); + fireEvent.change(escalateToInput(l), { target: { value: 'sre_lead' } }); + (screen.getByLabelText(l.notify) as HTMLInputElement).click(); + + const onConfig = configOf(latest())!; + const authored = { enabled: true, timeoutHours: 24, action: 'reassign', escalateTo: 'sre_lead', notifySubmitter: false }; + expect(onConfig.escalation, 'the inspector stored exactly what the author entered').toEqual(authored); + const on = ApprovalNodeConfigSchema.safeParse(onConfig); + expect(on.success, on.success ? '' : JSON.stringify(on.error.issues)).toBe(true); + expect((on.data as { escalation?: unknown }).escalation, 'and the contract keeps every value').toEqual(authored); + + // OFF. + gateBox(l)!.click(); + const offConfig = configOf(latest())!; + expect('escalation' in offConfig, 'switching off leaves no block').toBe(false); + expect(offConfig).toEqual({ approvers: APPROVERS }); + const off = ApprovalNodeConfigSchema.safeParse(offConfig); + expect(off.success, off.success ? '' : JSON.stringify(off.error.issues)).toBe(true); + expect(off.data as Record, 'the parsed node carries no escalation either').not.toHaveProperty('escalation'); + }); + }); + + describe(`${source.name} descriptors: a stored switched-off block from older metadata (objectui#11660)`, () => { + beforeEach(source.install); + + it('`{ enabled: false, timeoutHours: 24 }` reads OFF, shows the retained value, and writes nothing on render', () => { + // A shape the spec ACCEPTS — the old inspector wrote it when an author + // switched off a filled block. It is read, never rewritten on sight. + expect(ApprovalNodeConfigSchema.safeParse({ approvers: APPROVERS, escalation: { enabled: false, timeoutHours: 24 } }).success).toBe(true); + const draft = draftWith({ approvers: APPROVERS, escalation: { enabled: false, timeoutHours: 24 } }); + const before = JSON.stringify(draft); + const { patches } = mount(draft); + expect(gateBox(l)!.checked).toBe(false); + expect(shownSubFields(l), 'the stored value is re-shown (objectui#6499)').toEqual([l.timeout]); + expect(retainedNotices(), 'and flagged as kept, not in effect').toHaveLength(1); + expect(patches).toEqual([]); + expect(JSON.stringify(draft)).toBe(before); + }); + + it('clearing its last retained value leaves no `{ enabled: false }` stub behind', () => { + const { latest } = mount(draftWith({ approvers: APPROVERS, escalation: { enabled: false, timeoutHours: 24 } })); + fireEvent.click(within(retainedNotices()[0]).getByRole('button', { name: /clear value/i })); + const after = configOf(latest())!; + expect('escalation' in after, 'the bare stub the spec refuses is not written').toBe(false); + expect(ApprovalNodeConfigSchema.safeParse(after).success).toBe(true); + }); + + it('clearing ONE of two retained values keeps the other — only the bare stub is removed', () => { + // The boundary of the stub rule: it removes a block holding nothing but + // the switch, never a value the author did not clear. + const { latest } = mount( + draftWith({ approvers: APPROVERS, escalation: { enabled: false, timeoutHours: 24, escalateTo: 'sre_lead' } }), + ); + expect(retainedNotices(), 'premise: both stored values are retained').toHaveLength(2); + fireEvent.change(escalateToInput(l), { target: { value: '' } }); + expect(configOf(latest())!.escalation).toEqual({ enabled: false, timeoutHours: 24 }); + }); + }); +} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.inactiveRetained.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.inactiveRetained.test.tsx index 86b5c8f8c3..b820869831 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.inactiveRetained.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.inactiveRetained.test.tsx @@ -175,8 +175,12 @@ describe('clearing is deliberate, author-initiated, and goes through the ordinar const clear = within(notices()[0]).getByRole('button', { name: /clear value/i }); fireEvent.click(clear); const patched = onPatch.mock.calls.at(-1)![0] as any; - // The retained key is gone; the controller the author actually set stays. - expect(patched.nodes[0].config.escalation).toEqual({ enabled: false }); + // The retained key is gone. This row used to go on "the controller the + // author actually set stays" and pin `{ enabled: false }` — the stub + // `ApprovalNodeConfigSchema` refuses for its missing `timeoutHours`. + // objectui#11660: the escalation block's EXISTENCE is its switch, so the + // author's OFF survives as no block, the one spelling of it the spec admits. + expect(patched.nodes[0].config?.escalation).toBeUndefined(); }); it('does not clear anything until the author clicks — rendering is a read', () => { 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 5aa4488e05..1e1eefbeb4 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx @@ -37,6 +37,9 @@ import { isFieldVisible, inactiveRetainedKind, getFieldValue, + readFieldValue, + switchedBlockOf, + isBareSwitchedOffBlock, configKeyOf, FLOW_NODE_TYPE_OPTIONS, type FlowConfigField, @@ -365,7 +368,22 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, nextEdges = syncDecisionEdgesByOrder(node.id, value, draftEdges); } } - let nextNode = setAtPath(node, path, stored); + // objectui#11660 — a block whose EXISTENCE is its switch (`switchedBlockOf`: + // the approval node's SLA escalation). Switching it off removes the block, + // and every value stored in it with it — triage's ruling: no + // `enabled: false` stub, no `timeoutHours` kept behind a disabled toggle. + // Any other write that leaves the bare `{ enabled: false }` stub (clearing + // the last value retained under a stored switched-off block) removes it as + // well. `setAtPath` deletes only the block — sibling config keys are not + // rebuilt — and prunes a `config` left empty. + const switched = switchedBlockOf(node, field); + let nextNode = + switched?.isSwitch && stored === false + ? setAtPath(node, [...switched.block], undefined) + : setAtPath(node, path, stored); + if (switched && isBareSwitchedOffBlock(nextNode, switched)) { + nextNode = setAtPath(nextNode, [...switched.block], undefined); + } // 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); @@ -489,7 +507,9 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, // #4305 — show this repeater only the keys the typed sibling // fields do not own; `setField` merges its commit back. ? connectorInputExtras(getFieldValue(node, effField), effField.omitKeys) - : getFieldValue(node, effField); + // objectui#11660 — `readFieldValue`: a block switch over an + // absent block draws OFF, not its declared default. + : readFieldValue(node, effField); return ( = { // `flow-node-config.spec-reconciliation.test.ts` derives every default in // this block from the installed `ApprovalEscalationSchema`, so the next // upstream flip reddens there rather than diverging silently again. + // + // That default applies INSIDE a block. Whether the block exists is the + // switch itself (objectui#11660, `BLOCK_SWITCHES` below): no block reads + // off, and switching off removes the block — so the engine-published copy + // of this gate behaves the same, it is keyed by path, not declared here. { id: 'escalation.enabled', path: ['config', 'escalation', 'enabled'], label: 'SLA escalation', kind: 'boolean', defaultValue: 'true', help: 'Escalate when a decision is not recorded within the timeout.' }, { id: 'escalation.timeoutHours', path: ['config', 'escalation', 'timeoutHours'], label: 'Timeout (hours)', kind: 'number', placeholder: '24', showWhen: { field: 'escalation.enabled', equals: ['true'] } }, { @@ -1332,6 +1337,109 @@ export function getFieldValue(node: Record | null | undefined, return field.fallbackPath ? read(field.fallbackPath) : undefined; } +/** + * Boolean gates whose spec meaning is "the enclosing block EXISTS" + * (objectui#11660), keyed by node type and the gate's path. + * + * The approval node's SLA escalation is the one instance, and the spec states + * it in the declaration itself: `ApprovalEscalationSchema.enabled` is + * described as "the feature-level switch is whether the escalation block + * exists at all", while the block's `timeoutHours` is REQUIRED whatever + * `enabled` says. So the only spelling of OFF every reader accepts is NO + * BLOCK. The inspector used to write `{ enabled: false }` instead — a block + * `ApprovalNodeConfigSchema` refuses for its missing `timeoutHours`, so the + * flow saved and then failed at the approval node on every run. + * + * Keyed by PATH, not by a descriptor member: the offline table and the + * engine-published `configSchema` (`jsonSchemaToFlowFields`) both emit this + * path for the gate, so the rule holds whichever of the two produced the field + * on screen. Triage's ruling on objectui#11660 is what this list carries out: + * switching escalation off removes the block — no `enabled: false` stub, and + * no `timeoutHours` kept behind a disabled toggle. + */ +const BLOCK_SWITCHES: ReadonlyArray<{ nodeType: string; path: readonly string[] }> = [ + { nodeType: 'approval', path: ['config', 'escalation', 'enabled'] }, +]; + +/** Where a field sits relative to a {@link BLOCK_SWITCHES} block. */ +export interface SwitchedBlock { + /** The block whose existence is the switch, e.g. `['config', 'escalation']`. */ + block: readonly string[]; + /** The switch's own key inside that block, e.g. `enabled`. */ + switchKey: string; + /** Whether the field IS the switch, rather than a value stored inside the block. */ + isSwitch: boolean; +} + +/** + * The switched block `field` belongs to on `node` — as its switch or as a + * value inside it — or `undefined` for every other field (objectui#11660). + */ +export function switchedBlockOf( + node: Record | null | undefined, + field: Pick, +): SwitchedBlock | undefined { + const type = node?.type; + if (typeof type !== 'string') return undefined; + for (const s of BLOCK_SWITCHES) { + if (s.nodeType !== type) continue; + const block = s.path.slice(0, -1); + if (field.path.length <= block.length || !block.every((seg, i) => field.path[i] === seg)) continue; + const isSwitch = + field.kind === 'boolean' && field.path.length === s.path.length && s.path.every((seg, i) => field.path[i] === seg); + return { block, switchKey: s.path[s.path.length - 1], isSwitch }; + } + return undefined; +} + +function blockAt(node: Record | null | undefined, block: readonly string[]): unknown { + let cur: unknown = node; + for (const seg of block) { + if (cur && typeof cur === 'object' && !Array.isArray(cur)) cur = (cur as Record)[seg]; + else return undefined; + } + return cur; +} + +const isPlainRecord = (v: unknown): v is Record => + !!v && typeof v === 'object' && !Array.isArray(v); + +/** + * The value a field's control draws and a `showWhen` controller resolves + * through: the stored value ({@link getFieldValue}), except that a block + * switch over an ABSENT block reads `false` (objectui#11660). + * + * The switch's declared default (`'true'`, the spec's `.default(true)`) is what + * the runtime applies to an `enabled` key omitted from a block that EXISTS — + * the objectui#6620 reading, unchanged: `{ timeoutHours: 24 }` draws checked. + * With no block there is nothing for that default to apply to, and the spec + * calls the block's absence OFF; reading the default there drew the toggle + * checked, and revealed the four escalation fields, on every approval node + * that has no escalation at all. + * + * A READ: it writes nothing, so a node that stores no block keeps storing none. + */ +export function readFieldValue(node: Record | null | undefined, field: FlowConfigField): unknown { + const switched = switchedBlockOf(node, field); + if (switched?.isSwitch && !isPlainRecord(blockAt(node, switched.block))) return false; + return getFieldValue(node, field); +} + +/** + * Whether `node` holds `switched`'s block as the bare `{ : false }` + * stub — OFF spelt as a block, which `ApprovalNodeConfigSchema` refuses for its + * missing `timeoutHours` and which absence already says (objectui#11660). + */ +export function isBareSwitchedOffBlock( + node: Record | null | undefined, + switched: SwitchedBlock, +): boolean { + const block = blockAt(node, switched.block); + if (!isPlainRecord(block)) return false; + const keys = Object.keys(block); + return keys.length === 1 && keys[0] === switched.switchKey && block[switched.switchKey] === false; +} + /** * The `config` key this field owns, or `undefined` for fields stored outside * `config` (spec-structured blocks, top-level `timeoutMs`). Used by the @@ -1438,7 +1546,9 @@ function controllerAdmits( if (!field.showWhen) return true; const controller = fields.find((f) => f.id === field.showWhen!.field); if (!controller) return false; - const raw = getFieldValue(node, controller); + // `readFieldValue`, not the bare stored read: a block switch over an absent + // block resolves OFF rather than through its declared default (objectui#11660). + const raw = readFieldValue(node, controller); 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; From 161e28cf48d59615b405d50a378ab9894fc4f9eb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 18:25:53 +0000 Subject: [PATCH 2/4] refactor(app-shell): name the escalation-block object guard locally, not after a spec export (objectui#11660) `check:spec-symbols` refused `isPlainRecord`: `@objectstack/spec/data` exports a symbol of that name, and a local declaration under it reads as the spec's own definition. Renamed to `isBlockObject`; behaviour unchanged. Claude-Session: https://claude.ai/code/session_015W8GBu6sBiqus2L2xjMsAL Co-authored-by: Claude --- .../src/views/metadata-admin/inspectors/flow-node-config.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 f8b40bf497..01f6cfb4e6 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 @@ -1401,7 +1401,7 @@ function blockAt(node: Record | null | undefined, block: readon return cur; } -const isPlainRecord = (v: unknown): v is Record => +const isBlockObject = (v: unknown): v is Record => !!v && typeof v === 'object' && !Array.isArray(v); /** @@ -1421,7 +1421,7 @@ const isPlainRecord = (v: unknown): v is Record => */ export function readFieldValue(node: Record | null | undefined, field: FlowConfigField): unknown { const switched = switchedBlockOf(node, field); - if (switched?.isSwitch && !isPlainRecord(blockAt(node, switched.block))) return false; + if (switched?.isSwitch && !isBlockObject(blockAt(node, switched.block))) return false; return getFieldValue(node, field); } @@ -1435,7 +1435,7 @@ export function isBareSwitchedOffBlock( switched: SwitchedBlock, ): boolean { const block = blockAt(node, switched.block); - if (!isPlainRecord(block)) return false; + if (!isBlockObject(block)) return false; const keys = Object.keys(block); return keys.length === 1 && keys[0] === switched.switchKey && block[switched.switchKey] === false; } From 7f87a5d7ae1cc45020ec66454249652f091fd363 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 18:50:43 +0000 Subject: [PATCH 3/4] test(app-shell): render the escalation select's default-placeholder case on an existing block (objectui#11660) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The select-half sweep rendered every declaring field on an EMPTY config, which drew `approval.escalation.action` only because the escalation switch read its default over no block. No block now reads off and hides the block's fields, so that one case renders on `{ timeoutHours: 24 }` — the smallest block the spec accepts, with `action` still unset. Claude-Session: https://claude.ai/code/session_015W8GBu6sBiqus2L2xjMsAL Co-authored-by: Claude --- .../FlowNodeInspector.declaredDefault.test.tsx | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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 768719ba0e..16f7255268 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 @@ -1070,7 +1070,15 @@ describe('the declaration surface this card names', () => { ]); for (const c of cases) { - renderInspector(draftWith(c.type, { config: {} })); + // objectui#11660 — a field stored inside the approval escalation block is + // on screen only while that block EXISTS: the spec makes the block itself + // the switch, so no block reads off and hides its fields. Its case renders + // on the smallest block the spec accepts, `{ timeoutHours: 24 }` (the + // #6620 rows' fixture), whose own `enabled` is omitted and so reads on — + // the key under measurement is still unset. Every other case keeps the + // empty config. + const config = c.id.startsWith('approval.escalation.') ? { escalation: { timeoutHours: 24 } } : {}; + renderInspector(draftWith(c.type, { config })); expect( triggerText(c.label), `${c.id}: an unset key states the declared default on the trigger`, From 2f7d3984ecd0941811242e1559b054c2e06692bc Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 22:07:21 +0000 Subject: [PATCH 4/4] fix(app-shell): switching SLA escalation off keeps entered values beside enabled: false (objectui#11660) Triage amended the direction under objectui#6499 Option C. Switching off no longer removes a block that holds values: the ordinary write stores `enabled: false` beside them, every value kept, including a block with no `timeoutHours` yet (nothing filled in, nothing deleted). The only removal left is the bare `{ enabled: false }` stub `ApprovalNodeConfigSchema` refuses, so switching off with nothing entered, or clearing the last retained value, writes no block. No block still reads off. The pins flip to match: the full-block switch-off keeps every value byte-identical and parses; the round trip covers on and off-with-values; the nothing-entered, emptied-config, no-hours edge and rule-3-then-Clear cases are pinned on both descriptor sources. The changeset and docs drop the claim that no block is the only conforming off. Claude-Session: https://claude.ai/code/session_015W8GBu6sBiqus2L2xjMsAL Co-authored-by: Claude --- .../11660-escalation-off-removes-block.md | 13 +- ...NodeInspector.escalationOff-11660.test.tsx | 116 +++++++++++++----- ...lowNodeInspector.inactiveRetained.test.tsx | 6 +- .../inspectors/FlowNodeInspector.tsx | 22 ++-- .../inspectors/flow-node-config.ts | 37 ++++-- 5 files changed, 128 insertions(+), 66 deletions(-) diff --git a/.changeset/11660-escalation-off-removes-block.md b/.changeset/11660-escalation-off-removes-block.md index e019bbe876..8bb56bf217 100644 --- a/.changeset/11660-escalation-off-removes-block.md +++ b/.changeset/11660-escalation-off-removes-block.md @@ -2,14 +2,15 @@ '@object-ui/app-shell': patch --- -Switching an approval node's SLA escalation off in the flow designer removes the node's `escalation` block (objectui#11660). +The flow designer's SLA escalation switch on an approval node no longer writes a refused `escalation: { enabled: false }` block, and switching it off keeps the values the author entered (objectui#11660). -The inspector used to write `escalation: { enabled: false }` when an author turned the switch off. `ApprovalNodeConfigSchema` refuses that block, because `timeoutHours` is required whether or not `enabled` is `false`. So the flow saved and then failed at the approval node on every run, and once the platform checks node config at save time, the save itself is refused. The spec states what the switch means: "the feature-level switch is whether the escalation block exists at all". So off is now no block: +`ApprovalNodeConfigSchema` requires `escalation.timeoutHours` whether or not `enabled` is `false`. So `{ enabled: false, timeoutHours: 24 }` is a valid way to switch escalation off, and the runtime skips escalation for it. Having no block at all is also valid. The bare `{ enabled: false }` is not. The inspector wrote that bare block when an author switched off a node with nothing entered, because the switch drew on over a node that had no block. The flow saved and then failed at the approval node on every run, or, once the platform judges the block at save time, the save is refused. -- **Switching off removes the block**, with every value stored in it. The node's other config keys are not touched. A `config` left empty is dropped, as it is for any other cleared field. -- **No block reads off.** A node without an `escalation` block used to draw the switch on, with its four fields revealed, because the declared default (`true`) was applied with nothing to apply it to. It now draws the switch off. A block that omits `enabled` still reads on, and a stored `enabled: false` still reads off. -- **No bare `{ enabled: false }` block is written.** Clearing the last value kept under an older switched-off block removes the block too. A block that still holds another value is kept as it is. +- **No block reads off.** A node without an `escalation` block used to draw the switch on, with its four fields showing. The inspector applied the declared default (`true`) although there was no block for it to apply to. It now draws the switch off, and rendering writes nothing. A block that omits `enabled` still reads on, and a stored `enabled: false` still reads off. +- **Switching off with nothing entered writes no block.** Switching on writes `{ enabled: true }` alone. Switching back off before anything is entered removes the block instead of writing `{ enabled: false }`. A `config` left empty is dropped, as it is for any other cleared field. +- **Switching off a block that holds values keeps them.** The block gets `enabled: false`, and every entered value stays exactly as stored. They show as kept but not in effect, with a Clear action (objectui#6499). A block that has values but no `timeoutHours` yet keeps them too: no hours are filled in, and the block stays refused at `escalation.timeoutHours` until hours are entered, as it was before the switch. +- **Clearing the last kept value removes the block**, instead of leaving `{ enabled: false }` behind. A block that still holds another value is kept. -Both field sources behave the same: the offline field table and the `configSchema` the engine publishes for the approval node. A stored `{ enabled: false, timeoutHours: 24 }` from older metadata still reads off, shows its value as kept but not in effect, and is not rewritten when the node is opened. +The offline field table and the `configSchema` the engine publishes for the approval node behave the same. A stored block is never rewritten just because the node is opened. **Clause-②: no.** Nothing on the package entry changes. The new helpers (`switchedBlockOf`, `readFieldValue`, `isBareSwitchedOffBlock`) live in the inspector's own module, which `@object-ui/app-shell` does not export. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.escalationOff-11660.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.escalationOff-11660.test.tsx index d8451970b9..118ec69c38 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.escalationOff-11660.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.escalationOff-11660.test.tsx @@ -1,17 +1,29 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * objectui#11660 — switching an approval node's SLA escalation OFF removes the - * `escalation` block; it never writes the `{ enabled: false }` stub. + * objectui#11660 — the SLA escalation switch never writes the bare + * `{ enabled: false }` block `ApprovalNodeConfigSchema` refuses, and switching + * it off keeps every value the author entered. * * The governing text is the spec's own: `ApprovalEscalationSchema.enabled` is - * described as "the feature-level switch is whether the escalation block exists - * at all", and the block's `timeoutHours` is required whatever `enabled` says. - * Triage ruled the direction (comment 6000059097 on the card): turning - * escalation off removes the block — no `enabled: false` stub, no - * `timeoutHours` kept behind a disabled toggle — and the reading follows from - * it: no block reads OFF, a block that omits `enabled` reads ON (the - * objectui#6620 pin), a stored `enabled: false` reads OFF. + * described as "an escalation block carrying timeoutHours is live unless this + * is explicitly false — the feature-level switch is whether the escalation + * block exists at all", and the block's `timeoutHours` is required whatever + * `enabled` says. So `{ enabled: false, timeoutHours: N }` and no block are both + * a conforming OFF; the bare stub is not. Triage's amended direction (comment + * 6003792818 on the card, replacing 6000059097 under objectui#6499 Option C): + * + * 1. no block reads OFF, and rendering writes nothing; + * 2. switching off with no entered value writes no block; + * 3. switching off a block that holds values writes `enabled: false` and keeps + * every value — the "inactive values retained" display carries them; + * 4. clearing the last retained value removes the block; + * 5. a block with values but no `timeoutHours` keeps its values on switch-off: + * nothing filled in, nothing deleted, and its refusal is neither created + * nor hidden by the switch. + * + * A block that omits `enabled` reads ON (the objectui#6620 pin), and a stored + * `enabled: false` reads OFF. * * Every case runs twice, once per DESCRIPTOR SOURCE, because the inspector * renders `serverFields ?? fieldsForNodeType(...)`: @@ -215,10 +227,10 @@ for (const source of SOURCES) { }); }); - describe(`${source.name} descriptors: switching escalation off removes the block (objectui#11660)`, () => { + describe(`${source.name} descriptors: switching escalation off (objectui#11660, triage 6003792818)`, () => { beforeEach(source.install); - it('a block holding every value leaves no `escalation` key, and every sibling config key byte-identical', () => { + it('rule 3: a block holding every value gains `enabled: false`, every value and every sibling key byte-identical', () => { const siblings: Config = { approvers: APPROVERS, behavior: 'quorum', @@ -238,36 +250,72 @@ for (const source of SOURCES) { gateBox(l)!.click(); - const after = configOf(latest())!; - expect('escalation' in after, 'no `escalation` key is left behind — not a stub, not a hidden block').toBe(false); - // Byte-identical siblings, key order included: the removal deletes one key - // and rebuilds nothing else. - expect(JSON.stringify(after)).toBe(JSON.stringify(siblings)); - - // What the author sees next: the switch off, nothing revealed, and nothing - // flagged as retained — there is nothing left to retain. - expect(gateBox(l)!.checked, 'the toggle reads the absent block as off').toBe(false); - expect(shownSubFields(l)).toEqual([]); - expect(retainedNotices()).toHaveLength(0); + const { escalation, ...rest } = configOf(latest())!; + // Byte-identical, key order included: the switch is the one key that moved. + expect(JSON.stringify(escalation)).toBe( + JSON.stringify({ enabled: false, timeoutHours: 24, action: 'reassign', escalateTo: 'sre_lead', notifySubmitter: false }), + ); + expect(JSON.stringify(rest), 'the sibling config keys are untouched').toBe(JSON.stringify(siblings)); + const parsed = ApprovalNodeConfigSchema.safeParse({ approvers: APPROVERS, escalation }); + expect(parsed.success, parsed.success ? '' : JSON.stringify(parsed.error.issues)).toBe(true); + + // What the author sees next: the switch off, and the four values on screen + // as retained, not in effect (objectui#6499 Option C). + expect(gateBox(l)!.checked, 'the stored false reads off').toBe(false); + expect(shownSubFields(l)).toEqual(subLabels(l)); + expect(retainedNotices()).toHaveLength(4); }); - it('a block that omits `enabled` (switch drawn on by default) is removed too', () => { + it('rule 3: a block that omits `enabled` keeps its value and gains `enabled: false`', () => { const { latest } = mount(draftWith({ approvers: APPROVERS, escalation: { timeoutHours: 8 } })); gateBox(l)!.click(); - expect(configOf(latest())).toEqual({ approvers: APPROVERS }); + expect(configOf(latest())).toEqual({ approvers: APPROVERS, escalation: { timeoutHours: 8, enabled: false } }); + }); + + it('rule 2: switching on writes only the switch, and switching off with nothing entered leaves no `escalation` key', () => { + const { latest } = mount(draftWith({ approvers: APPROVERS })); + gateBox(l)!.click(); + // Nothing the author did not enter is written by switching on: no + // `action`, no `notifySubmitter` default (shown, never written). + expect(configOf(latest())!.escalation, 'switching on writes the switch alone').toEqual({ enabled: true }); + gateBox(l)!.click(); + expect(configOf(latest()), 'no `{ enabled: false }` stub is written').toEqual({ approvers: APPROVERS }); + expect(gateBox(l)!.checked).toBe(false); }); - it('an emptied `config` is pruned with the block', () => { - const { latest } = mount(draftWith({ escalation: { enabled: true, timeoutHours: 24 } })); + it('rule 2: an emptied `config` is pruned with the block', () => { + const { latest } = mount(draftWith({ escalation: { enabled: true } })); gateBox(l)!.click(); expect(latest().nodes[0], 'removing the only config key leaves no empty `config` object').not.toHaveProperty('config'); }); + + it('rule 5: a block with values but no `timeoutHours` keeps them, gains no `timeoutHours`, and stays refused where it was', () => { + const before: Config = { approvers: APPROVERS, escalation: { enabled: true, action: 'reassign' } }; + const refusedAt = (cfg: Config) => { + const r = ApprovalNodeConfigSchema.safeParse(cfg); + return r.success ? [] : r.error.issues.map((i) => i.path.join('.')); + }; + expect(refusedAt(before), 'premise: refused at the missing hours before the switch').toEqual(['escalation.timeoutHours']); + const { latest } = mount(draftWith(before)); + gateBox(l)!.click(); + const after = configOf(latest())!; + expect(after.escalation, 'the value stays; nothing is filled in').toEqual({ enabled: false, action: 'reassign' }); + expect(refusedAt(after), 'the switch neither creates nor hides that refusal').toEqual(['escalation.timeoutHours']); + }); + + it('rule 4 on a block rule 3 wrote: clearing its last retained value leaves no block', () => { + const { latest } = mount(draftWith({ approvers: APPROVERS, escalation: { enabled: true, timeoutHours: 24 } })); + gateBox(l)!.click(); + expect(configOf(latest())!.escalation, 'premise: rule 3 kept the hours').toEqual({ enabled: false, timeoutHours: 24 }); + fireEvent.click(within(retainedNotices()[0]).getByRole('button', { name: /clear value/i })); + expect(configOf(latest())).toEqual({ approvers: APPROVERS }); + }); }); describe(`${source.name} descriptors: an inspector-authored node round-trips through ApprovalNodeConfigSchema (objectui#11660)`, () => { beforeEach(source.install); - it('escalation ON with every value parses and keeps every value; OFF leaves no block and parses', async () => { + it('escalation ON with every value parses and keeps every value; OFF keeps every value beside `enabled: false` and parses', async () => { const { latest } = mount(draftWith({ approvers: APPROVERS })); expect(gateBox(l)!.checked, 'premise: a fresh node starts off').toBe(false); @@ -287,23 +335,23 @@ for (const source of SOURCES) { expect(on.success, on.success ? '' : JSON.stringify(on.error.issues)).toBe(true); expect((on.data as { escalation?: unknown }).escalation, 'and the contract keeps every value').toEqual(authored); - // OFF. + // OFF, values retained. gateBox(l)!.click(); const offConfig = configOf(latest())!; - expect('escalation' in offConfig, 'switching off leaves no block').toBe(false); - expect(offConfig).toEqual({ approvers: APPROVERS }); + const retained = { ...authored, enabled: false }; + expect(JSON.stringify(offConfig.escalation), 'every value identical, the switch false').toBe(JSON.stringify(retained)); const off = ApprovalNodeConfigSchema.safeParse(offConfig); expect(off.success, off.success ? '' : JSON.stringify(off.error.issues)).toBe(true); - expect(off.data as Record, 'the parsed node carries no escalation either').not.toHaveProperty('escalation'); + expect((off.data as { escalation?: unknown }).escalation, 'and the contract keeps them, switched off').toEqual(retained); }); }); - describe(`${source.name} descriptors: a stored switched-off block from older metadata (objectui#11660)`, () => { + describe(`${source.name} descriptors: a stored switched-off block (objectui#11660)`, () => { beforeEach(source.install); it('`{ enabled: false, timeoutHours: 24 }` reads OFF, shows the retained value, and writes nothing on render', () => { - // A shape the spec ACCEPTS — the old inspector wrote it when an author - // switched off a filled block. It is read, never rewritten on sight. + // A shape the spec ACCEPTS, and the one rule 3 writes. It is read, never + // rewritten on sight. expect(ApprovalNodeConfigSchema.safeParse({ approvers: APPROVERS, escalation: { enabled: false, timeoutHours: 24 } }).success).toBe(true); const draft = draftWith({ approvers: APPROVERS, escalation: { enabled: false, timeoutHours: 24 } }); const before = JSON.stringify(draft); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.inactiveRetained.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.inactiveRetained.test.tsx index b820869831..357c15ef65 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.inactiveRetained.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.inactiveRetained.test.tsx @@ -178,8 +178,10 @@ describe('clearing is deliberate, author-initiated, and goes through the ordinar // The retained key is gone. This row used to go on "the controller the // author actually set stays" and pin `{ enabled: false }` — the stub // `ApprovalNodeConfigSchema` refuses for its missing `timeoutHours`. - // objectui#11660: the escalation block's EXISTENCE is its switch, so the - // author's OFF survives as no block, the one spelling of it the spec admits. + // objectui#11660 (triage 6003792818, rule 4): with its last retained value + // cleared, the block would hold nothing but its switch, and the author's + // OFF survives as no block — which the spec reads as off too. A block that + // still held another value would be kept, `enabled: false` and all. expect(patched.nodes[0].config?.escalation).toBeUndefined(); }); 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 1e1eefbeb4..3e9fcf287d 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx @@ -369,18 +369,18 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, } } // objectui#11660 — a block whose EXISTENCE is its switch (`switchedBlockOf`: - // the approval node's SLA escalation). Switching it off removes the block, - // and every value stored in it with it — triage's ruling: no - // `enabled: false` stub, no `timeoutHours` kept behind a disabled toggle. - // Any other write that leaves the bare `{ enabled: false }` stub (clearing - // the last value retained under a stored switched-off block) removes it as - // well. `setAtPath` deletes only the block — sibling config keys are not - // rebuilt — and prunes a `config` left empty. + // the approval node's SLA escalation). The write itself is the ordinary + // one, so switching off a block that holds entered values stores + // `enabled: false` beside them, every value kept (objectui#6499 Option C, + // triage 6003792818) — including a block that has no `timeoutHours` yet: + // nothing is filled in, nothing deleted. What is never stored is the bare + // `{ enabled: false }` stub `ApprovalNodeConfigSchema` refuses: a write + // that would leave it — switching off with nothing entered, or clearing the + // last value retained under a switched-off block — removes the block + // instead, which says the same OFF. `setAtPath` deletes only the block — + // sibling config keys are not rebuilt — and prunes a `config` left empty. const switched = switchedBlockOf(node, field); - let nextNode = - switched?.isSwitch && stored === false - ? setAtPath(node, [...switched.block], undefined) - : setAtPath(node, path, stored); + let nextNode = setAtPath(node, path, stored); if (switched && isBareSwitchedOffBlock(nextNode, switched)) { nextNode = setAtPath(nextNode, [...switched.block], undefined); } 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 01f6cfb4e6..1bb9442058 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 @@ -996,8 +996,10 @@ const FLOW_NODE_CONFIG: Record = { // // That default applies INSIDE a block. Whether the block exists is the // switch itself (objectui#11660, `BLOCK_SWITCHES` below): no block reads - // off, and switching off removes the block — so the engine-published copy - // of this gate behaves the same, it is keyed by path, not declared here. + // off, and switching off never writes a bare `{ enabled: false }` — with + // nothing entered no block is written; entered values stay beside + // `enabled: false`. The engine-published copy of this gate behaves the + // same: the rule is keyed by path, not declared here. { id: 'escalation.enabled', path: ['config', 'escalation', 'enabled'], label: 'SLA escalation', kind: 'boolean', defaultValue: 'true', help: 'Escalate when a decision is not recorded within the timeout.' }, { id: 'escalation.timeoutHours', path: ['config', 'escalation', 'timeoutHours'], label: 'Timeout (hours)', kind: 'number', placeholder: '24', showWhen: { field: 'escalation.enabled', equals: ['true'] } }, { @@ -1343,19 +1345,25 @@ export function getFieldValue(node: Record | null | undefined, * * The approval node's SLA escalation is the one instance, and the spec states * it in the declaration itself: `ApprovalEscalationSchema.enabled` is - * described as "the feature-level switch is whether the escalation block - * exists at all", while the block's `timeoutHours` is REQUIRED whatever - * `enabled` says. So the only spelling of OFF every reader accepts is NO - * BLOCK. The inspector used to write `{ enabled: false }` instead — a block - * `ApprovalNodeConfigSchema` refuses for its missing `timeoutHours`, so the - * flow saved and then failed at the approval node on every run. + * described as "an escalation block carrying timeoutHours is live unless this + * is explicitly false — the feature-level switch is whether the escalation + * block exists at all", and the block's `timeoutHours` is REQUIRED whatever + * `enabled` says. So OFF has two conforming spellings: NO BLOCK, and a block + * that keeps its `timeoutHours` beside `enabled: false` (the runtime skips the + * escalation sweep on an explicit `false`). What it refuses is the bare + * `{ enabled: false }` stub — and the inspector used to write exactly that when + * an author switched off a node with nothing entered, because the switch drew + * ON over a node with no block. The flow saved and then failed at the approval + * node on every run, or, at a door that judges the block, did not save. * * Keyed by PATH, not by a descriptor member: the offline table and the * engine-published `configSchema` (`jsonSchemaToFlowFields`) both emit this * path for the gate, so the rule holds whichever of the two produced the field - * on screen. Triage's ruling on objectui#11660 is what this list carries out: - * switching escalation off removes the block — no `enabled: false` stub, and - * no `timeoutHours` kept behind a disabled toggle. + * on screen. Triage's ruling on objectui#11660 (6003792818, amending its + * first grade under objectui#6499 Option C) is what this list carries out: no + * block reads off; switching off with nothing entered writes no block; + * switching off a block holding values writes `enabled: false` and keeps every + * value; clearing the last retained value removes the block. */ const BLOCK_SWITCHES: ReadonlyArray<{ nodeType: string; path: readonly string[] }> = [ { nodeType: 'approval', path: ['config', 'escalation', 'enabled'] }, @@ -1427,8 +1435,11 @@ export function readFieldValue(node: Record | null | undefined, /** * Whether `node` holds `switched`'s block as the bare `{ : false }` - * stub — OFF spelt as a block, which `ApprovalNodeConfigSchema` refuses for its - * missing `timeoutHours` and which absence already says (objectui#11660). + * stub — OFF spelt as a block that holds no entered value, which + * `ApprovalNodeConfigSchema` refuses for its missing `timeoutHours` and which + * absence already says (objectui#11660). A switched-off block that still holds + * any other key is NOT a stub: its values are the author's, kept (objectui#6499 + * Option C). */ export function isBareSwitchedOffBlock( node: Record | null | undefined,