diff --git a/.changeset/empty-predicate-declared-gate-3850-3862.md b/.changeset/empty-predicate-declared-gate-3850-3862.md new file mode 100644 index 0000000000..bdeac72435 --- /dev/null +++ b/.changeset/empty-predicate-declared-gate-3850-3862.md @@ -0,0 +1,74 @@ +--- +"@object-ui/core": patch +"@object-ui/components": patch +"@object-ui/react": patch +--- + +An empty predicate is no longer a declared gate anywhere (objectui#3850, objectui#3862) + +"Is a gate DECLARED on this key — is there a condition to reach a verdict on?" was +answered three times in this repo, with three different scopes, and the widest +answers sat on `disabled`, where the mistake is not benign: + +- `hasDeclaredVisibilityGate` (the action face) asked `!= null && !== ''`, so every + OBJECT counted — including `{ dialect: 'cel', source: '' }`. That envelope is not + a hand-written curiosity: `@objectstack/spec`'s `ExpressionInputSchema` normalizes + every authored predicate into one, so "the author left the predicate empty" + compiles to exactly it. The verdict path normalized the same value back to + `undefined`, and `evaluateCondition(undefined)` answers `true` — "no condition, so + visible/enabled". On `visible` that `true` means SHOW, so the two mistakes + cancelled; on `disabled` it means GREY, so they compounded: a button disabled + forever that no author asked to disable (objectui#3850, the residue objectui#3842 + left behind). +- `SchemaRenderer` asked `disabled !== undefined` inline, one notch wider again, so + `disabled: null` greyed out too — on the GENERIC rendering path, since that block + runs for every node type, and not as an internal flag either: `_disabled` is + forwarded to the component as a real `disabled` prop (objectui#3862). +- `ActionRunner`'s execution gates asked "does this normalize to something + evaluable?" — the scope that turned out to be right (objectui#3848 / objectui#3872). + +There is now ONE definition, `hasDeclaredPredicate`, exported from +`@object-ui/core` (`evaluator/declaredPredicate.ts`, beside the `toPredicateInput` +normalizer it is derived from): a gate is declared when normalization still leaves a +condition to evaluate. `''`, a whitespace-only string, an empty-`source` envelope +and any non-predicate value (`0`, `{}`) are NOT declared; `false` IS (a verdict is +not a missing gate — objectui#3812). `hasDeclaredVisibilityGate` keeps its name as a +re-export of it, so the five member-action renderer call sites, `DeclaredActionsBar` +and `record-quick-actions` are unchanged and inherit the scope; +`SchemaRenderer`'s `disabled` / `disabledOn` chain and `ActionRunner`'s two gates +read the same function. No consumer got a local "and also check for empty" test — +that fourth dialect is what objectui#3842 / objectui#3849 spent two PRs merging away. + +Measured behaviour change, `action:button` and the generic path, before → after: + +| value | `visible` | `disabled` | `enabled` | `SchemaRenderer` `disabled` prop | +|---|---|---|---|---| +| `''` | shown → shown | on → on | on → on | forwarded → absent | +| `null` | shown → shown | on → on | on → on | forwarded → absent | +| `{ dialect: 'cel', source: '' }` | shown → shown | GREY → on | on → on | forwarded → absent | +| `{ source: '' }` | shown → shown | GREY → on | on → on | forwarded → absent | +| `' '` (whitespace) | HIDDEN → shown | on → on | GREY → on | forwarded → absent | +| `0` / `{}` (not predicates) | shown → shown | GREY → on | on → on | forwarded → absent | +| `true` / `false` / bare CEL / `${…}` / non-empty envelope | unchanged | unchanged | unchanged | unchanged | + +Every row moves toward "there is no gate here", never away from it, and no value +that HAS a verdict changes it — the verdict is still read from the raw value, only +the gate in front of it narrowed. Two rows are behaviour changes rather than the +equivalence the ruling expected, and are pinned as such: the whitespace string moves +on `visible` / `enabled` (it used to normalize to `'${ }'`, which evaluates falsy, +so a predicate that says nothing HID the action from everyone), and non-predicate +junk stops greying controls out (fail-open, the posture `ActionRunner` already +committed to). + +One blank spelling is knowingly still outside the scope: an envelope whose `source` +is blank but not EMPTY (`{ dialect: 'cel', source: ' ' }`) — the normalizer folds a +`source` of `''` and does not trim, so the string spelling of a blank predicate is +trimmed and the envelope spelling is not, and `disabled` still greys out for that one +value. The ruling enumerated three empty spellings; this is a fourth, measured and +filed as objectui#3960 rather than widened in here. + +One chain is deliberately untouched: `SchemaRenderer`'s `visible` / `visibleWhen` / +`visibleOn` / `visibility` / `hidden` / `hiddenOn` legs keep `!== undefined`, because +narrowing them would change ALIAS PRECEDENCE, not just emptiness. The `hidden` legs +are not negated and therefore carry this same defect with the polarity that makes the +node vanish — measured, out of this ruling's scope, filed as objectui#3955. diff --git a/packages/components/src/index.ts b/packages/components/src/index.ts index 280c2d8057..0c848c8f27 100644 --- a/packages/components/src/index.ts +++ b/packages/components/src/index.ts @@ -42,9 +42,12 @@ export { cva } from 'class-variance-authority'; export { getLazyIcon, isLucideIconName, LazyIcon, toKebabIconName } from './lib/lazy-icon'; // The member-action visibility gate — "did this action DECLARE a `visible` gate -// at all?" (`!= null && !== ''`), the single definition objectui#3492 -// established and PR #3816 / #3825 / #3836 applied to every member-action gate -// in this package and in `plugin-grid`. +// at all?", the single definition objectui#3492 established and PR #3816 / +// #3825 / #3836 applied to every member-action gate in this package and in +// `plugin-grid`. Since objectui#3850 the answer is "normalization still leaves a +// condition to evaluate" (so an empty-`source` envelope is NOT a gate, where the +// older `!= null && !== ''` counted every object), and this name is a re-export +// of core's one definition, `hasDeclaredPredicate`. // // Exported because the family has a member OUTSIDE these packages: app-shell's // `DeclaredActionsBar` mounts an object's server-declared actions as plain JSX diff --git a/packages/components/src/renderers/action/__tests__/action-empty-predicate-scope.test.tsx b/packages/components/src/renderers/action/__tests__/action-empty-predicate-scope.test.tsx new file mode 100644 index 0000000000..bf0d3fdcf1 --- /dev/null +++ b/packages/components/src/renderers/action/__tests__/action-empty-predicate-scope.test.tsx @@ -0,0 +1,257 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#3850 — how wide "empty predicate" is on the action face, now that + * `hasDeclaredVisibilityGate` is a re-export of core's one definition + * (`hasDeclaredPredicate`, `evaluator/declaredPredicate.ts`) instead of a + * second, narrower answer to the same question. + * + * The residue objectui#3842 left behind: `hasDeclaredVisibilityGate` asked + * `!= null && !== ''`, so every OBJECT counted as a declared gate — including + * `{ dialect: 'cel', source: '' }`, the shape `@objectstack/spec`'s + * `ExpressionInputSchema` produces when an author leaves a predicate empty, and + * therefore the empty shape most likely to reach a renderer from real metadata. + * The verdict path normalized the same value to `undefined`, and + * `evaluateCondition(undefined)` answers `true` = "no condition → visible / + * enabled". On `visible` that `true` means SHOW, so the two mistakes cancelled; + * on `disabled` it means GREY, so they compounded — a button disabled forever + * that no author asked to disable. + * + * ## What each case detects + * + * • the `disabled` leg's two envelope cases (`{ dialect, source: '' }` and + * `{ source: '' }`) → clickable. THE defect, one case per row of the #3850 + * table's "残留" lines. Restore `!= null && !== ''` in the definition and + * exactly these go red. + * • the `disabled` leg's junk cases (`0`, `{}`) → clickable. Behaviour change + * in the same fail-open direction: a value the evaluator cannot read must not + * be the reason a control is dead. (`ActionRunner` already committed this + * module family to `catch { isDisabled = false }`.) + * • `disabled: true` / a truthy expression / a truthy CEL envelope → still + * greyed. Anti-mutation guards: "never disable anything" satisfies most of + * this file on its own, and these refuse it. + * • `disabled: false` and no `disabled` at all → still clickable, so widening + * "empty" did not swallow a declared-and-false verdict (objectui#3812). + * • the `visible` leg's empty cases → still rendered, which is the EQUIVALENCE + * the objectui#3850 ruling asked for on that family. Two of the three rows + * are equivalence in the strict sense (`''` and the envelope reached "shown" + * before this change too, by cancellation); the whitespace row is NOT — see + * the next block, which pins the change rather than hiding it. + * + * ## The whitespace row moves, and this suite says so out loud + * + * Measured on this tree, `action:button` before → after: + * + * | value | `visible` | `disabled` | `enabled` | + * |--------------------------------|----------------|-------------|------------| + * | `''` | shown → shown | on → on | on → on | + * | `{ dialect:'cel', source:'' }` | shown → shown | GREY → on | on → on | + * | `{ source: '' }` | shown → shown | GREY → on | on → on | + * | `' '` (whitespace only) | HIDDEN → shown | on → on | GREY → on | + * | `0` / `{}` | shown → shown | GREY → on | on → on | + * + * `' '` used to be a declared gate whose verdict came from `'${ }'` (the + * normalizer wraps a whitespace string rather than folding it), which evaluates + * falsy — so a predicate that says nothing HID the action from everyone, and + * greyed it out through the negated `enabled` leg. Moving it to "no gate" is the + * direction this gate's own doc has always claimed ("an empty predicate must not + * hide the action from everyone either"); it is a behaviour CHANGE on `visible` + * and `enabled` all the same, so it is pinned as one. + * + * ## Why one leaf plus an identity assertion covers five call sites + * + * `action:button`, `action:icon`, `action:group`'s inline button and dropdown + * item, and `action:menu`'s item all import the same symbol from + * `../visibility-gate` — unchanged by this move, which is the point of keeping + * the name. The identity case below asserts that symbol IS core's + * `hasDeclaredPredicate`, so the scope pinned here is the scope all five read; a + * leaf that re-spelled the question locally would break that claim, not hide + * behind it. The `''` rows for the member leaves stay where objectui#3842 / + * objectui#3849 put them (`action-member-disabled-declared-gate.test.tsx`). + */ + +import { describe, it, expect } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; +import { ComponentRegistry, hasDeclaredPredicate } from '@object-ui/core'; +import { PredicateScopeProvider } from '@object-ui/react'; +// Module-scope side-effect import so the renderer is in the registry when +// `ComponentRegistry.get` runs (the light `dom` project does not load the +// `@object-ui/components` graph), per AGENTS.md §测试纪律. +import '../action-button'; +import { hasDeclaredVisibilityGate } from '../visibility-gate'; + +/** Mount the leaf the way `action:bar` mounts it: whole action spread onto `schema`. */ +function renderLeaf(action: Record, scope: Record = {}) { + const Renderer = ComponentRegistry.get('action:button'); + if (!Renderer) throw new Error('action:button is not registered'); + return render( + + + , + ); +} + +const ACT = { name: 'act', label: 'Act', type: 'script' }; +const act = () => screen.getByRole('button', { name: 'Act' }); + +/** The shapes with nothing to evaluate, by the spelling that produces them. */ +const EMPTY_SHAPES: Array<{ label: string; value: unknown }> = [ + { label: "'' (empty string)", value: '' }, + { label: "' ' (whitespace only)", value: ' ' }, + { label: "{ dialect: 'cel', source: '' } (what `objectstack build` emits)", value: { dialect: 'cel', source: '' } }, + { label: "{ source: '' } (envelope without a dialect)", value: { source: '' } }, + { label: 'null', value: null }, +]; + +/** Values the evaluator cannot read at all. */ +const JUNK_SHAPES: Array<{ label: string; value: unknown }> = [ + { label: '0', value: 0 }, + { label: '{} (an object with no source)', value: {} }, +]; + +describe('the one definition — `hasDeclaredVisibilityGate` is core\'s `hasDeclaredPredicate` (objectui#3850)', () => { + it('is the same function object, not a same-shaped copy', () => { + // A re-spelled twin here is how this question grew three scopes in the first + // place (objectui#3142 is what copies of one answer cost). Identity, so the + // five member-action call sites and this suite cannot drift apart. + expect(hasDeclaredVisibilityGate).toBe(hasDeclaredPredicate); + }); + + it('answers the empty spellings and the junk with "no gate", and a declared boolean with "gate"', () => { + for (const { label, value } of [...EMPTY_SHAPES, ...JUNK_SHAPES]) { + expect(hasDeclaredVisibilityGate(value), `${label} must not count as a declared gate`).toBe(false); + } + expect(hasDeclaredVisibilityGate(false)).toBe(true); + expect(hasDeclaredVisibilityGate(true)).toBe(true); + }); +}); + +describe('action:button `disabled` — an empty predicate is not a gate (objectui#3850)', () => { + it.each(EMPTY_SHAPES)('disabled: $label → the button stays clickable', ({ value }) => { + renderLeaf({ ...ACT, disabled: value }); + expect(act()).not.toBeDisabled(); + }); + + it.each(JUNK_SHAPES)('disabled: $label (not a predicate) → the button stays clickable', ({ value }) => { + renderLeaf({ ...ACT, disabled: value }); + expect(act()).not.toBeDisabled(); + }); + + it('disabled: true → still greyed out', () => { + renderLeaf({ ...ACT, disabled: true }); + expect(act()).toBeDisabled(); + }); + + it('disabled: false → still clickable (a verdict, not a missing gate)', () => { + renderLeaf({ ...ACT, disabled: false }); + expect(act()).not.toBeDisabled(); + }); + + it('no `disabled` at all → still clickable', () => { + renderLeaf({ ...ACT }); + expect(act()).not.toBeDisabled(); + }); + + it("disabled: { dialect: 'cel', source: 'true' } → still greyed out (a NON-empty envelope is a gate)", () => { + renderLeaf({ ...ACT, disabled: { dialect: 'cel', source: 'true' } }); + expect(act()).toBeDisabled(); + }); + + it('an expression-valued `disabled` keeps its verdict, both ways', () => { + const gated = { ...ACT, disabled: 'features.locked == true' }; + const { unmount } = renderLeaf(gated, { features: { locked: true } }); + expect(act()).toBeDisabled(); + unmount(); + renderLeaf(gated, { features: { locked: false } }); + expect(act()).not.toBeDisabled(); + }); +}); + +describe('action:button `visible` — the equivalence the ruling asked for, and the one row that moves', () => { + it.each(EMPTY_SHAPES)('visible: $label → the action is still rendered', ({ value }) => { + renderLeaf({ ...ACT, visible: value }); + expect(act()).toBeInTheDocument(); + }); + + it.each(JUNK_SHAPES)('visible: $label (not a predicate) → still rendered', ({ value }) => { + renderLeaf({ ...ACT, visible: value }); + expect(act()).toBeInTheDocument(); + }); + + it('visible: false → still hidden, and visible: true / a true predicate still shown', () => { + const { container, unmount } = renderLeaf({ ...ACT, visible: false }); + expect(container.querySelector('button')).toBeNull(); + unmount(); + renderLeaf({ ...ACT, visible: true }); + expect(act()).toBeInTheDocument(); + }); + + it('a false expression still hides it (the gate narrowed; evaluation did not change)', () => { + const { container } = renderLeaf({ ...ACT, visible: 'features.beta == true' }, { features: { beta: false } }); + expect(container.querySelector('button')).toBeNull(); + }); + + it("CHANGED: visible: ' ' was HIDDEN before this ruling and is now shown", () => { + // Not equivalence — the honest row. `' '` normalized to `'${ }'`, which + // evaluates falsy, so a predicate that says nothing hid the action from + // everyone. See this file's header table. + renderLeaf({ ...ACT, visible: ' ' }); + expect(act()).toBeInTheDocument(); + }); +}); + +describe('action:button legacy `enabled` leg — same definition, same scope', () => { + it.each(EMPTY_SHAPES)('enabled: $label → not greyed out', ({ value }) => { + renderLeaf({ ...ACT, enabled: value }); + expect(act()).not.toBeDisabled(); + }); + + it('CHANGED: enabled: \' \' greyed the button out before this ruling', () => { + // The negated leg (`disabled = !isEnabled`) turned the whitespace string's + // falsy verdict into "disabled". Covered by the row above; stated separately + // because it is a behaviour change, not a preserved one. + renderLeaf({ ...ACT, enabled: ' ' }); + expect(act()).not.toBeDisabled(); + }); + + it('enabled: false → still greyed out (unchanged)', () => { + renderLeaf({ ...ACT, enabled: false }); + expect(act()).toBeDisabled(); + }); + + it('enabled: true → not greyed out (unchanged)', () => { + renderLeaf({ ...ACT, enabled: true }); + expect(act()).not.toBeDisabled(); + }); + + it('`disabled` still wins over `enabled` when it is declared', () => { + renderLeaf({ ...ACT, disabled: true, enabled: true }); + expect(act()).toBeDisabled(); + }); + + it("an EMPTY `disabled` no longer short-circuits the chain — `enabled: false` is reached", () => { + // The precedence case: with `disabled: ''` no longer a gate, the chain falls + // through to the legacy leg instead of stopping at an empty predicate. + renderLeaf({ ...ACT, disabled: '', enabled: false }); + expect(act()).toBeDisabled(); + }); + + it('an empty ENVELOPE `disabled` falls through the same way (objectui#3850 row)', () => { + const { unmount } = renderLeaf({ ...ACT, disabled: { dialect: 'cel', source: '' }, enabled: false }); + expect(act()).toBeDisabled(); + unmount(); + // The other direction is the mutation detector of the pair: under the old + // scope the envelope was a declared gate whose verdict was `true`, so the + // button was greyed out no matter what `enabled` said. + renderLeaf({ ...ACT, disabled: { dialect: 'cel', source: '' }, enabled: true }); + expect(act()).not.toBeDisabled(); + }); +}); diff --git a/packages/components/src/renderers/action/visibility-gate.ts b/packages/components/src/renderers/action/visibility-gate.ts index ec53164a30..d215fed640 100644 --- a/packages/components/src/renderers/action/visibility-gate.ts +++ b/packages/components/src/renderers/action/visibility-gate.ts @@ -7,39 +7,76 @@ */ /** - * Did this action DECLARE a visibility gate at all? The one definition read by - * every member-action leaf on the action face — `action:group`'s inline button - * and dropdown item, and `action:menu`'s item — so the three cannot drift into - * three answers for one key (the shape objectui#3142 already had to unpick for - * `locations` in these same files). - * - * Truthiness cannot answer this question, and asking it is the objectui#3812 - * defect: `if (action.visible && !isVisible)` classified `visible: false` — the - * most explicit way an author can say "never show this" — as *ungated*, so the - * gate never consulted the verdict and the action rendered for everyone. - * - * `!= null && !== ''` is not a new decision. objectui#3492 established it for - * the selection bar, where `plugin-grid`'s `hasVisibilityGate` spells out the - * same reasoning verbatim ("Truthiness cannot answer this: `visible: false` is - * a declared gate that excludes everything"); objectui#3758 / PR #3816 applied - * it to both row-action surfaces (`isCustomRowActionVisible` in - * `plugin-grid/src/components/RowActionMenu.tsx` and - * `renderers/complex/data-table.tsx`). - * - * This function decides only whether a gate EXISTS. The verdict is the - * declaration's own business and is left to the evaluation entry, which already - * short-circuits a boolean instead of handing it to the CEL engine: - * `useCondition(toPredicateInput(false))` is `false` and - * `useCondition(toPredicateInput(true))` is `true`, pinned for both the engine - * and the renderer path in - * `packages/react/src/hooks/__tests__/actionPredicate.parity.test.tsx`. Nothing - * about that entry changes here — only the gate in front of it, which refused - * to ask. - * - * `''` is grouped with `null`/`undefined` deliberately: an empty predicate is - * nothing to evaluate, so it must not hide the action from everyone either. - * `toPredicateInput` maps it to `undefined` (visible) for the same reason. + * Did this action DECLARE a visibility gate at all? The renderer-side name for + * the repo's one definition of that question, which now lives one layer down in + * `@object-ui/core` as `hasDeclaredPredicate` + * (`packages/core/src/evaluator/declaredPredicate.ts`) — read the reasoning + * there; only what is specific to this file is repeated below. + * + * ## Why this module is now a re-export (objectui#3850 / objectui#3862) + * + * The question was answered three times with three different scopes, and the + * widest-scoped answer was on the key where the mistake is not benign: + * + * - HERE, `!= null && !== ''` — so every OBJECT counted as a declared gate, + * including `{ dialect: 'cel', source: '' }`. That envelope is not an exotic + * spelling: `@objectstack/spec`'s `ExpressionInputSchema` normalizes every + * authored predicate into one, so "the author left the predicate empty" + * compiles to exactly it. The verdict path then normalized the same value to + * `undefined` and `evaluateCondition(undefined)` answered `true` = "no + * condition, so visible/enabled" — which on `disabled` means GREYED OUT. + * A button disabled forever, indistinguishable from the metadata's intent + * (objectui#3850). + * - `SchemaRenderer`'s inline `!== undefined`, one notch wider again, so + * `disabled: null` greyed out any node on the generic rendering path + * (objectui#3862). + * - `ActionRunner`'s module-private helper, which asked "does this normalize + * to something evaluable?" — the scope objectui#3850 then ruled to be THE + * scope (objectui#3848). + * + * The ruling: a gate is declared when normalization still leaves a condition to + * evaluate. `''`, a whitespace-only string, an empty-`source` envelope and any + * non-predicate value are therefore NOT declared, and one definition in core + * says so for the renderers, `SchemaRenderer` and the execution entry alike. + * + * The NAME is kept (`hasDeclaredVisibilityGate`) and this module keeps + * re-exporting it: the five member-action call sites — `action:button`, + * `action:icon`, `action:group`'s inline button and dropdown item, and + * `action:menu`'s item, plus `DeclaredActionsBar` and `record-quick-actions` + * through the `@object-ui/components` barrel — are unchanged by this move. The + * name is historic (objectui#3492 arrived through `visible`) and the predicate is + * key-neutral, which the call-site comments already say. + * + * ## What did NOT change: truthiness still cannot answer this + * + * `if (action.visible && !isVisible)` was the objectui#3812 defect: it + * classified `visible: false` — the most explicit way an author can say "never + * show this" — as *ungated*, so the gate never consulted the verdict and the + * action rendered for everyone. A declared boolean is still a declared gate + * here; only the empty and non-predicate shapes moved. + * + * ## The scope change is not verdict-neutral on every key — measured + * + * On `visible` the empty shapes were mostly benign already, because the two + * mistakes cancelled: over-broad "declared" plus `evaluateCondition` answering + * `true` for an empty predicate = SHOWN, the same result as "no gate". Measured + * on this tree (`action:button`, before → after): + * + * | value | `visible` | `disabled` | `enabled` | + * |------------------------------|-------------------|-------------------|-------------------| + * | `''` | shown → shown | on → on | on → on | + * | `{ dialect:'cel', source:'' }`| shown → shown | GREY → on | on → on | + * | `{ source: '' }` | shown → shown | GREY → on | on → on | + * | `' '` (whitespace) | HIDDEN → shown | on → on | GREY → on | + * | `0` / `{}` (not a predicate) | shown → shown | GREY → on | on → on | + * | `true` / `false` / CEL / `${…}`| unchanged | unchanged | unchanged | + * + * The whitespace row is the one that moves on `visible`, and it moves toward the + * invariant rather than away from it: `' '` used to be normalized to `'${ }'` + * (the normalizer wraps it rather than folding it), which evaluates falsy, so a + * predicate that says nothing HID the action from everyone. "An empty predicate + * must not hide the action from everyone" is what this gate has always claimed — + * the whitespace spelling simply never obeyed it. Same story on the negated + * `enabled` leg, where it greyed the control out instead. */ -export function hasDeclaredVisibilityGate(visible: unknown): boolean { - return visible != null && visible !== ''; -} +export { hasDeclaredPredicate as hasDeclaredVisibilityGate } from '@object-ui/core'; diff --git a/packages/core/src/actions/ActionRunner.ts b/packages/core/src/actions/ActionRunner.ts index b8083afea1..3724cbb738 100644 --- a/packages/core/src/actions/ActionRunner.ts +++ b/packages/core/src/actions/ActionRunner.ts @@ -24,7 +24,7 @@ import type { RunnableActionType } from '@object-ui/types'; import type { ActionInput as SpecActionInput } from '@objectstack/spec/ui'; import { ExpressionEvaluator } from '../evaluator/ExpressionEvaluator'; -import { toPredicateInput } from '../evaluator/predicateInput'; +import { hasDeclaredPredicate } from '../evaluator/declaredPredicate'; import { globalUndoManager, type UndoableOperation } from './UndoManager'; import { warnOnDeprecatedObjectParams, warnOnUnknownActionKeys } from './actionKeys'; @@ -536,19 +536,19 @@ function withIdentityAlias(context: ActionContext): ActionContext { return { ...context, os: { ...(os && typeof os === 'object' ? os : {}), user } }; } -/** - * Did this action declare a gate at all — i.e. is there a CONDITION for the - * evaluator to reach a verdict on? (objectui#3848 for `disabled`, objectui#3872 - * for `condition`) - * - * ## One definition, two gates — why this is not two helpers +/* + * ## The two predicate gates in `execute`, and the one question they ask first * - * `execute` has two predicate gates, and both need this ONE question answered - * before evaluating. They need it for OPPOSITE reasons, which is exactly why the - * question has to be asked separately from the verdict: + * `execute` gates on `condition` and on `disabled`, and both need ONE question + * answered before evaluating — "is a gate DECLARED here, i.e. is there a + * condition to reach a verdict on?" — for OPPOSITE reasons, which is exactly why + * the question has to be asked separately from the verdict: * * - `disabled` (objectui#3848): an EMPTY predicate must not be handed to * `evaluateCondition`, whose answer for "no condition" is `true` = BLOCKED. + * Measured consequence of asking `!= null && !== false` instead: + * `disabled: ''` returned `{ success: false, error: 'Action is disabled' }` + * and the handler never ran. * - `condition` (objectui#3872): a DECLARED-FALSE predicate must not be * skipped by a truthiness test. `if (action.condition)` never asked whether * a gate was declared, so `condition: false` — the most explicit "never @@ -560,49 +560,28 @@ function withIdentityAlias(context: ActionContext): ActionContext { * short-circuits booleans (`if (typeof condition === 'boolean') return * condition`) and blocks. * - * The scope of "empty predicate" below is objectui#3850's ruling (`''` / - * whitespace-only / empty-`source` envelope are NOT declared), so both gates - * read the same one. A second module-private twin of the same question in this - * one file is the drift the closing scope note refuses; the name is therefore - * key-neutral, and the `disabled` gate's semantics and message are unchanged by - * objectui#3872. - * - * `ExpressionEvaluator.evaluateCondition` documents and implements one default - * for "there is no condition here": it returns `true`, meaning - * *visible/enabled*. That default is correct on `visible` / `enabled`, and - * INVERTED on `disabled`, where `true` means "blocked". So the execution gate - * must decide "is there a condition?" itself, BEFORE evaluating — asking - * `!= null && !== false` (what it used to ask) hands every empty predicate to a - * function whose answer for "nothing to evaluate" is the strongest possible - * "yes, disabled". Measured consequence: `disabled: ''` returned - * `{ success: false, error: 'Action is disabled' }` and the handler never ran. - * - * The shapes `evaluateCondition` itself calls "no condition" are `null` / - * `undefined` / `''` / a whitespace-only string (`if (!trimmed) return true`) / - * an envelope whose `source` is blank. This gate excludes exactly those: + * Both read {@link hasDeclaredPredicate} — the repo's one definition of that + * question, in `evaluator/declaredPredicate.ts` beside the normalizer it is + * derived from, with the scope objectui#3850 ruled on (`''` / whitespace-only / + * empty-`source` envelope / non-predicate junk are NOT declared; a declared + * `false` IS). This gate arrived here first as a module-private helper with a + * scope note saying it stayed private until that ruling landed; it has, so the + * definition moved down a layer and the renderer side reads the same one + * (`components/renderers/action/visibility-gate.ts` re-exports it as + * `hasDeclaredVisibilityGate`, `SchemaRenderer`'s `disabled` chain calls it). * - * - `toPredicateInput` is core's single predicate normalizer and already maps - * `''`, `null`, `undefined`, an empty-`source` envelope, and any - * non-predicate junk (`0`, `{}`) to `undefined` = "nothing to evaluate". - * - the whitespace-only string is the one shape it does NOT collapse (it - * wraps it as `'${ }'`), so it is named here. This is the same blank-source - * rule core's other predicate entry already applies — - * `evalRowPredicate` (`evaluator/listConditional.ts`) returns its fallback - * for `!source.trim()`. + * ## Why the gates normalize to DECIDE but evaluate the RAW value * - * ## Why the gate normalizes but the VERDICT still reads the raw value - * - * Historically the two were NOT interchangeable for a string already spelled as + * Historically the two were not interchangeable for a string already spelled as * a `${…}` template: `toPredicateInput` assumed a bare expression and wrapped * unconditionally, so `'${x}'` became `'${${x}}'`, which no longer matched the * single-template fast path and did not parse — leaving a constant verdict whose * direction was set by the caller's error policy (fail-soft got the unparsed * string back, `Boolean(…)` = `true`; fail-closed got a throw). On `disabled` - * that meant "always blocked", on `condition` the opposite ("always execute"), - * so both gates here normalize only to decide DECLAREDNESS and evaluate the raw - * value. That defect was objectui#3871, fixed at the normalizer: an - * already-`${…}` string is now returned untouched, the normalizer is idempotent, - * and `evaluateCondition(toPredicateInput(raw))` agrees with + * that meant "always blocked", on `condition` the opposite ("always execute"). + * That defect was objectui#3871, fixed at the normalizer: an already-`${…}` + * string is returned untouched, the normalizer is idempotent, and + * `evaluateCondition(toPredicateInput(raw))` agrees with * `evaluateCondition(raw)` on every shape these gates accept. * * The gates still read the raw value. Nothing forces the change now that the two @@ -610,33 +589,18 @@ function withIdentityAlias(context: ActionContext): ActionContext { * reason has shifted from "must" to "no reason to", so the equality is pinned * next to each gate's suite (`ActionRunner.disabledGate.test.ts` / * `ActionRunner.conditionGate.test.ts`, the two cases that replaced the - * objectui#3871 tripwires) instead of being assumed. - * - * So the value handed to the evaluator is left exactly as it was. Every - * non-empty shape — boolean, bare CEL, `${…}` template, `{ dialect, source }` - * envelope — keeps the verdict it reaches today, and this change can only stop - * blocking things, never start. - * - * Scope note: `hasDeclaredVisibilityGate` - * (`components/renderers/action/visibility-gate.ts`, `!= null && !== ''`) is - * the renderer-side spelling of this question. It is deliberately NOT reused or - * moved: it lives in a package that depends on this one, it does not cover the - * empty-`source` envelope (objectui#3850) or the whitespace string, and - * objectui#3850 is the queued ruling on unifying the scope of "empty - * predicate" across the sites. Kept module-private until that lands — one - * definition, one module, no new exported dialect of the same question. + * objectui#3871 tripwires) instead of being assumed. Every non-empty shape — + * boolean, bare CEL, `${…}` template, `{ dialect, source }` envelope — keeps the + * verdict it reaches today. * * Nor is the `visible`-filter template in `ActionEngine.getActionsForLocation` - * copied wholesale: its non-predicate branch keeps a historical - * `Boolean(raw)` coercion, so `0` there means "hidden" — fail-CLOSED on junk, - * the opposite of what this module already committed to for `disabled` + * copied wholesale: its non-predicate branch keeps a historical `Boolean(raw)` + * coercion, so `0` there means "hidden" — fail-CLOSED on junk, the opposite of + * what this module already committed to for `disabled` * (`catch { isDisabled = false }`). A value that is not a predicate at all must - * not decide an action's fate, on either key, so `0` / `{}` are "no gate" here. + * not decide an action's fate, on either key, so `0` / `{}` are "no gate" here + * and in the shared definition. */ -function hasDeclaredPredicate(value: unknown): boolean { - if (typeof value === 'string' && value.trim() === '') return false; - return toPredicateInput(value) !== undefined; -} export class ActionRunner { private handlers = new Map(); diff --git a/packages/core/src/actions/__tests__/ActionRunner.conditionGate.test.ts b/packages/core/src/actions/__tests__/ActionRunner.conditionGate.test.ts index 5f548cb665..aefc0707f1 100644 --- a/packages/core/src/actions/__tests__/ActionRunner.conditionGate.test.ts +++ b/packages/core/src/actions/__tests__/ActionRunner.conditionGate.test.ts @@ -295,7 +295,13 @@ describe('why the `condition` gate cannot ask truthiness (objectui#3872)', () => // does not copy that: `catch { isDisabled = false }` already committed this // module to fail-OPEN on junk, and a value that is not a predicate must not // decide an action's fate. Pinned so the next reader sees the difference is - // chosen, not overlooked — objectui#3850's follow-up owns unifying it. + // chosen, not overlooked. + // + // Ownership, corrected: objectui#3850 landed WITHOUT unifying this — its + // ruling covered the "declared?" definition and its placement (core, with + // the renderer face and `SchemaRenderer` reading it), not the engine filter's + // own scope. `ActionEngine` is now the last consumer answering this question + // with its own range, measured on both faces and filed as objectui#3957. const engine = new ActionEngine(CONTEXT); engine.registerAction( { name: 'junk_visible', type: 'script', target: '"ran"', visible: 0 } as unknown as ActionDef, diff --git a/packages/core/src/actions/__tests__/ActionRunner.disabledGate.test.ts b/packages/core/src/actions/__tests__/ActionRunner.disabledGate.test.ts index c111a2756a..9ede7bec52 100644 --- a/packages/core/src/actions/__tests__/ActionRunner.disabledGate.test.ts +++ b/packages/core/src/actions/__tests__/ActionRunner.disabledGate.test.ts @@ -43,23 +43,30 @@ * = false }`): a value that is not a predicate at all must not decide that * an action is disabled. * - * ## The parity claim, and the one row it deliberately does NOT make + * ## The parity claim, now made for EVERY row (objectui#3850 closed the last three) * * `rendererDisabled` is transcribed from objectui#3848's divergence table (the * action face as it stands after #3842 + #3849), NOT recomputed here: * `@object-ui/core` is the dependency of the renderer packages, so it cannot - * import them, and the live renderer-side pins are in those two PRs' tests. The + * import them, and the live renderer-side pins are in those PRs' tests. The * parity test below therefore asserts that the verdicts THIS suite pins equal * the verdicts recorded from the renderers — the #3314 invariant (one predicate - * value, one answer, whichever entry reads it) — for every row where that claim - * is honest. ONE row is excluded, with its owning issue: + * value, one answer, whichever entry reads it). * - * • `{ dialect: 'cel', source: '' }` — the renderer still greys this out - * (`hasDeclaredVisibilityGate` is `!= null && !== ''`, which an envelope - * passes). That residual is objectui#3850, the queued ruling on how wide - * "empty predicate" is across the sites; the execution side pins the - * normalized semantics (an envelope with no source is nothing to evaluate). - * Not fixed here — objectui#3850 owns the renderer half. + * Three rows used to be excluded, all of them the renderer's "is a gate + * DECLARED?" scope: `{ dialect: 'cel', source: '' }` (an envelope passes + * `!= null && !== ''`, so the renderer greyed the button out while this side read + * the same value as nothing to evaluate) and the two junk rows `0` / `{}` (same + * mechanism, coerced instead of folded). objectui#3850 ruled that scope — a gate + * is declared when normalization still leaves a condition — and the definition + * this file's gate asked privately moved into + * `evaluator/declaredPredicate.ts` as `hasDeclaredPredicate`, which the renderer + * side now re-exports as `hasDeclaredVisibilityGate`. So the three rows claim + * parity, backed by live renderer-side pins rather than transcription alone: + * `packages/components/src/renderers/action/__tests__/action-empty-predicate-scope.test.tsx` + * (the action leaf, all five empty spellings plus the junk rows) and + * `packages/react/src/__tests__/SchemaRenderer.disabledDeclaredGate.test.tsx` + * (the generic path, objectui#3862). * * The two `${…}`-spelled rows used to be excluded as well, and no longer are. * The action-face renderers compose `evaluateCondition(toPredicateInput(x))`, @@ -117,11 +124,13 @@ const SHAPES: Shape[] = [ { label: "disabled: '' (empty predicate)", disabled: '', blocked: false, rendererDisabled: false }, { label: "disabled: ' ' (whitespace-only predicate)", disabled: ' ', blocked: false, rendererDisabled: false }, { + // Parity claimed since objectui#3850 moved the "declared?" definition into + // core and the renderer side started reading it: an envelope with no source + // is nothing to evaluate on both faces. label: "disabled: { dialect: 'cel', source: '' } (empty envelope)", disabled: { dialect: 'cel', source: '' }, blocked: false, - rendererDisabled: null, - divergence: 'objectui#3850 — the renderer still reads an empty-source envelope as a declared gate', + rendererDisabled: false, }, // ── unchanged: ungated stays ungated ──────────────────────────────────── { label: 'disabled absent (undeclared)', absent: true, blocked: false, rendererDisabled: false }, @@ -167,8 +176,10 @@ const SHAPES: Shape[] = [ rendererDisabled: false, }, // ── behaviour change: non-predicate junk fails open ───────────────────── - { label: 'disabled: 0 (not a predicate)', disabled: 0, blocked: false, rendererDisabled: null, divergence: 'objectui#3850 — the renderers read a non-predicate as a declared gate and coerce it' }, - { label: 'disabled: {} (not a predicate)', disabled: {}, blocked: false, rendererDisabled: null, divergence: 'objectui#3850 — the renderers read a non-predicate as a declared gate and coerce it' }, + // Parity claimed since objectui#3850: the renderers stopped reading a + // non-predicate as a declared gate, so junk fails open on both faces. + { label: 'disabled: 0 (not a predicate)', disabled: 0, blocked: false, rendererDisabled: false }, + { label: 'disabled: {} (not a predicate)', disabled: {}, blocked: false, rendererDisabled: false }, ]; /** Execute one shape and report whether the handler ran. */ @@ -211,11 +222,12 @@ describe('ActionRunner.execute — declared `disabled` gate (objectui#3848)', () it('the execution verdict equals the renderer verdict for every shape where parity is claimed', () => { const claimed = SHAPES.filter(s => s.rendererDisabled !== null); - // Guard the guard: if a future edit nulls out the whole column, this test - // would pass by asserting nothing. Raised from 8 to 10 when objectui#3871 - // brought the two `${…}` rows into the claim — a floor that does not move - // with the table is a floor that stops guarding it. - expect(claimed.length).toBeGreaterThanOrEqual(10); + // Guard the guard: if a future edit nulls out the column, this test would + // pass by asserting nothing. 8 → 10 when objectui#3871 brought the two `${…}` + // rows into the claim, 10 → the whole table when objectui#3850 brought the + // envelope and junk rows in. A floor that does not move with the table is a + // floor that stops guarding it. + expect(claimed.length).toBe(SHAPES.length); for (const shape of claimed) { expect( shape.blocked, @@ -224,20 +236,22 @@ describe('ActionRunner.execute — declared `disabled` gate (objectui#3848)', () } }); - it('records which shapes are knowingly still divergent, and who owns each', () => { + it('no shape is exempt from the parity claim any more, and a future exemption must name its issue', () => { + // This replaces the "which rows are knowingly divergent" case, which after + // objectui#3850 would have asserted an EMPTY list — green because nothing is + // produced, not because the invariant holds. The claim now is the stronger + // one: every shape in the table is compared, and the `divergence` escape + // hatch stays exercised — re-introducing one without an owning issue fails + // here rather than quietly shrinking the comparison. const divergent = SHAPES.filter(s => s.rendererDisabled === null); - for (const shape of divergent) { - expect(shape.divergence, `${shape.label} must name the issue that owns it`).toMatch(/objectui#\d+/); + expect(divergent.map(s => s.label)).toEqual([]); + for (const shape of SHAPES) { + if (shape.rendererDisabled === null) { + expect(shape.divergence, `${shape.label} must name the issue that owns it`).toMatch(/objectui#\d+/); + } else { + expect(shape.divergence, `${shape.label} claims parity, so it must not also claim a divergence`).toBeUndefined(); + } } - // The two `${…}` rows left this list when objectui#3871 was fixed — the - // renderer face stopped answering with a constant, so there is no longer a - // difference to own. What remains is one family: the renderer's "is a gate - // DECLARED?" scope, which objectui#3850 owns for all three shapes. - expect(divergent.map(s => s.label)).toEqual([ - "disabled: { dialect: 'cel', source: '' } (empty envelope)", - 'disabled: 0 (not a predicate)', - 'disabled: {} (not a predicate)', - ]); }); }); diff --git a/packages/core/src/evaluator/__tests__/declaredPredicate.test.ts b/packages/core/src/evaluator/__tests__/declaredPredicate.test.ts new file mode 100644 index 0000000000..7707131913 --- /dev/null +++ b/packages/core/src/evaluator/__tests__/declaredPredicate.test.ts @@ -0,0 +1,156 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#3850 — the ONE definition of "is a predicate gate declared here?", + * now that it lives in core beside the normalizer it is derived from. The + * consumer-side pins are + * `packages/components/src/renderers/action/__tests__/action-empty-predicate-scope.test.tsx` + * (the action face, through the `hasDeclaredVisibilityGate` re-export), + * `packages/react/src/__tests__/SchemaRenderer.disabledDeclaredGate.test.tsx` + * (objectui#3862, the generic rendering path) and + * `packages/core/src/actions/__tests__/ActionRunner.disabledGate.test.ts` / + * `.conditionGate.test.ts` (the execution entry, which asked this scope first). + * + * ## What this suite pins that the consumers cannot + * + * The consumers can only observe the ANSWER through a rendered control. This one + * pins the scope itself, one shape per case, plus the two structural claims that + * keep the scope from drifting again: + * + * • DERIVATION: for every shape except the whitespace string, + * `hasDeclaredPredicate(x)` is exactly `toPredicateInput(x) !== undefined`. + * That equality is the reason this definition is trustworthy — "declared" + * means "normalization still leaves something to evaluate", not a hand-rolled + * list of empty spellings. A re-spelled predicate that happens to agree on + * today's shapes would pass the row cases and fail this one. + * • the whitespace string is the single deliberate DIFFERENCE from the + * normalizer (which wraps `' '` into `'${ }'` rather than folding it), + * asserted as such so the next reader sees it is chosen, not overlooked. + * • the one shape this scope does NOT cover — an envelope whose `source` is + * blank but not empty (`{ dialect: 'cel', source: ' ' }`) — is pinned as a + * documented residue with the issue that owns it (objectui#3960). It was + * found by this suite: the first draft assumed the normalizer folds it, and + * that assertion went red. `disabled` still greys out for that spelling, so + * it is recorded rather than left for the next reader to rediscover. + * + * ## Reverse verification (direction predicted before running) + * + * Narrowing the definition back to the renderer's historic scope + * (`value != null && value !== ''`) must turn RED exactly: the empty-envelope + * rows (both spellings), the whitespace row, the junk rows, and the derivation + * case. `''` / `null` / `undefined` / `true` / `false` / every non-empty + * expression row stays GREEN — they agreed under both scopes, which is why the + * envelope residue survived objectui#3842 in the first place. + */ + +import { describe, it, expect } from 'vitest'; +import { hasDeclaredPredicate } from '../declaredPredicate'; +import { toPredicateInput } from '../predicateInput'; +import { ExpressionEvaluator } from '../ExpressionEvaluator'; + +/** Every shape the #3850 table measured, plus the boolean verdicts. */ +const SHAPES: Array<{ label: string; value: unknown; declared: boolean }> = [ + // ── nothing to evaluate ──────────────────────────────────────────────── + { label: 'undefined (no key)', value: undefined, declared: false }, + { label: 'null', value: null, declared: false }, + { label: "'' (empty predicate)", value: '', declared: false }, + { label: "' ' (whitespace only)", value: ' ', declared: false }, + { label: "'\\t\\n' (other blanks)", value: '\t\n', declared: false }, + { label: "{ dialect: 'cel', source: '' } (the envelope `objectstack build` emits)", value: { dialect: 'cel', source: '' }, declared: false }, + { label: "{ source: '' } (envelope, no dialect)", value: { source: '' }, declared: false }, + // NOT here: `{ dialect: 'cel', source: ' ' }`. Measured as still DECLARED — + // objectui#3850's ruling enumerated three empty spellings and the blank-source + // envelope is a fourth. Pinned as a documented residue below (objectui#3960) + // rather than silently folded into this table. + // ── not a predicate at all → fail open, never a reason to disable ────── + { label: '0', value: 0, declared: false }, + { label: '{} (no source)', value: {}, declared: false }, + { label: '[] (array)', value: [], declared: false }, + { label: '{ dialect: "cel" } (envelope with no source key)', value: { dialect: 'cel' }, declared: false }, + // ── a declared verdict, including the explicit `false` ───────────────── + { label: 'true', value: true, declared: true }, + { label: 'false (a verdict, not a missing gate — objectui#3812)', value: false, declared: true }, + { label: 'bare CEL expression', value: 'user.role == "admin"', declared: true }, + { label: '`${…}` template', value: '${user.role === "admin"}', declared: true }, + { label: "{ dialect: 'cel', source: 'true' }", value: { dialect: 'cel', source: 'true' }, declared: true }, + { label: "{ dialect: 'template', source: '${x}' }", value: { dialect: 'template', source: '${x}' }, declared: true }, +]; + +describe('hasDeclaredPredicate — the scope objectui#3850 ruled on', () => { + it.each(SHAPES)('$label → declared=$declared', ({ value, declared }) => { + expect(hasDeclaredPredicate(value)).toBe(declared); + }); + + it('the three empty spellings the ruling names are one answer, not three', () => { + // `''` was objectui#3842's half, the envelope objectui#3850's, the + // whitespace string objectui#3848's. One definition, so they cannot diverge + // again. + expect([ + hasDeclaredPredicate(''), + hasDeclaredPredicate(' '), + hasDeclaredPredicate({ dialect: 'cel', source: '' }), + ]).toEqual([false, false, false]); + }); +}); + +describe('hasDeclaredPredicate is DERIVED from the normalizer, not re-spelled', () => { + it('agrees with `toPredicateInput(x) !== undefined` on every shape but the whitespace string', () => { + const disagreements = SHAPES.filter( + s => hasDeclaredPredicate(s.value) !== (toPredicateInput(s.value) !== undefined), + ).map(s => s.label); + expect(disagreements).toEqual([ + "' ' (whitespace only)", + "'\\t\\n' (other blanks)", + ]); + }); + + it('the whitespace string is the one shape the normalizer wraps instead of folding', () => { + // Which is why the definition names it explicitly. Same blank-source rule + // `evaluateCondition` (`if (!trimmed) return true`) and `evalRowPredicate` + // (`listConditional.ts`, `if (!source.trim())`) already apply. + expect(toPredicateInput(' ')).toBe('${ }'); + expect(toPredicateInput('')).toBeUndefined(); + expect(toPredicateInput({ dialect: 'cel', source: '' })).toBeUndefined(); + }); + + it('DOCUMENTED RESIDUE (objectui#3960): a BLANK envelope `source` is still declared', () => { + // The asymmetry, stated rather than hidden: this definition trims the STRING + // spelling and does not trim an envelope's `source`, because + // `toPredicateInput` only folds a source that is `''`: + expect(toPredicateInput({ dialect: 'cel', source: ' ' })).toEqual({ dialect: 'cel', source: ' ' }); + expect(hasDeclaredPredicate({ dialect: 'cel', source: ' ' })).toBe(true); + // …while core's own CEL entry calls exactly that value "no predicate": + const ev = new ExpressionEvaluator({ record: { id: 1 } }); + expect(ev.evaluateCondition({ dialect: 'cel', source: ' ' })).toBe(true); + // Declared gate + "no condition → true" is the objectui#3850 mechanism with + // the blank moved inside the envelope, so `disabled` still greys out for this + // one spelling. objectui#3850's ruling enumerated three empty spellings and + // this is a fourth, so it is filed (objectui#3960) instead of being widened + // into the ruled scope here. These expectations are what go RED when it is + // fixed — the signal to move the shape into the table above. + }); +}); + +describe('why the question cannot be delegated to the verdict', () => { + it('evaluateCondition answers `true` for "nothing to evaluate" — which on `disabled` means GREY', () => { + const ev = new ExpressionEvaluator({ record: { id: 1 } }); + // The inverted default, in four lines. Correct on `visible`/`enabled`, + // backwards on `disabled` — so every consumer has to ask "declared?" first. + expect(ev.evaluateCondition(undefined)).toBe(true); + expect(ev.evaluateCondition('')).toBe(true); + expect(ev.evaluateCondition(' ')).toBe(true); + expect(ev.evaluateCondition({ dialect: 'cel', source: '' })).toBe(true); + }); + + it('and truthiness cannot answer it either — `false` is a verdict (objectui#3812)', () => { + expect(hasDeclaredPredicate(false)).toBe(true); + const ev = new ExpressionEvaluator({}); + expect(ev.evaluateCondition(false)).toBe(false); + }); +}); diff --git a/packages/core/src/evaluator/declaredPredicate.ts b/packages/core/src/evaluator/declaredPredicate.ts new file mode 100644 index 0000000000..db1698d11f --- /dev/null +++ b/packages/core/src/evaluator/declaredPredicate.ts @@ -0,0 +1,98 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import { toPredicateInput } from './predicateInput'; + +/** + * Is a predicate gate DECLARED on this value — i.e. after normalization, is + * there still a CONDITION for {@link ExpressionEvaluator.evaluateCondition} to + * reach a verdict on? (objectui#3850's ruling, key-neutral: `visible` / + * `hidden` / `enabled` / `disabled` / `condition` all ask it.) + * + * This is the ONE definition of that question in the repo, and it lives here — + * one layer under every consumer, beside {@link toPredicateInput}, whose answer + * it is derived from. It has to be asked separately from the verdict because + * `evaluateCondition` documents and implements exactly one default for "there + * is nothing here to evaluate": it returns `true`, meaning *visible/enabled*. + * That default is right on `visible` / `enabled` and INVERTED on `disabled`, + * where `true` means "greyed out" — so a gate that hands an empty predicate to + * the evaluator gets the strongest possible "yes, disable it" for a value the + * metadata never used to say anything. Truthiness cannot answer it either, and + * asking it that way was objectui#3812: `if (action.visible && !isVisible)` + * read `visible: false` — the most explicit "never show this" an author can + * write — as *ungated*, and rendered the action for everyone. + * + * ## What counts as "nothing to evaluate" + * + * Exactly the shapes {@link toPredicateInput} folds to `undefined`, plus the one + * it wraps instead of folding: + * + * - `null` / `undefined` — no key at all. + * - `''` — an empty predicate (objectui#3492 / objectui#3842). + * - a whitespace-only string — the shape the normalizer does NOT collapse + * (`' '` becomes `'${ }'`), named here so it cannot slip through. Core's + * other predicate entries already treat it as blank: `evaluateCondition` + * (`if (!trimmed) return true`) and `evalRowPredicate` + * (`evaluator/listConditional.ts`, `if (!source.trim())`). + * - `{ dialect, source: '' }` — the empty ENVELOPE. This is not an exotic + * spelling: `@objectstack/spec`'s `ExpressionInputSchema` normalizes every + * authored predicate into an envelope, so "author left the predicate empty" + * compiles to exactly this, which makes it the likeliest empty shape in real + * metadata (objectui#3850). + * - anything that is not a predicate at all (`0`, `{}`, an array): a value the + * evaluator cannot read must not be the reason a control is disabled or an + * action refuses to run. Fail-open on junk, which is the posture + * `ActionRunner` already committed to (`catch { isDisabled = false }`). + * + * A declared-and-`false` gate is DECLARED (`toPredicateInput` returns the + * boolean unchanged): `disabled: false` / `visible: false` are verdicts, and + * routing them to the evaluator — which short-circuits booleans — is the point + * of asking "declared?" rather than "truthy?". + * + * ## The one blank shape this scope does NOT cover (objectui#3960) + * + * An envelope whose `source` is blank but not EMPTY — + * `{ dialect: 'cel', source: ' ' }` — is still "declared" here, because + * `toPredicateInput` folds a `source` of `''` and does not trim. So the string + * spelling of a blank predicate is trimmed and the envelope spelling is not, + * which means `disabled` still greys out for that one value while core's own CEL + * entry calls it "no predicate" (`evaluateCelCondition`: `if (!source.trim()) + * return true`). objectui#3850's ruling enumerated three empty spellings and this + * is a fourth, so it is filed rather than widened in here — the asymmetry is + * pinned in `__tests__/declaredPredicate.test.ts` so it cannot be mistaken for + * a covered case. + * + * ## Why the callers still evaluate the RAW value + * + * Every consumer normalizes to decide DECLAREDNESS and then evaluates whatever + * it was given, `evaluateCondition(raw)` or `evaluateCondition(toPredicateInput( + * raw))` as it did before. Both agree since objectui#3871 made the normalizer + * idempotent for an already-`${…}` string; before that they did not, and a + * template-spelled predicate normalized twice came back as a constant. Nothing + * here changes a verdict for a value that HAS one — this gate only decides + * whether there is one to reach. + * + * ## Scope history (objectui#3850) + * + * The question used to be answered three times with three different scopes: the + * renderer-side `hasDeclaredVisibilityGate` (`!= null && !== ''`, so every + * object counted — including the empty envelope), `SchemaRenderer`'s inline + * `!== undefined` (wider still: `disabled: null` greyed the control out, + * objectui#3862), and `ActionRunner`'s module-private helper (this scope, which + * arrived first with objectui#3848 and is what the ruling adopted). One + * definition with one scope replaces all three: + * `components/renderers/action/visibility-gate.ts` re-exports this function + * under its historic name `hasDeclaredVisibilityGate` (the five member-action + * renderer call sites are unchanged), `SchemaRenderer`'s `disabled` / + * `disabledOn` chain reads it, and `ActionRunner`'s two gates read it instead of + * a private twin. + */ +export function hasDeclaredPredicate(value: unknown): boolean { + if (typeof value === 'string' && value.trim() === '') return false; + return toPredicateInput(value) !== undefined; +} diff --git a/packages/core/src/evaluator/index.ts b/packages/core/src/evaluator/index.ts index 1f6b7e2e40..0a6d8dea1d 100644 --- a/packages/core/src/evaluator/index.ts +++ b/packages/core/src/evaluator/index.ts @@ -9,6 +9,7 @@ export * from './ExpressionContext.js'; export * from './ExpressionEvaluator.js'; export * from './predicateInput.js'; +export * from './declaredPredicate.js'; export * from './fieldRules.js'; export * from './listConditional.js'; export * from './optionRules.js'; diff --git a/packages/react/src/SchemaRenderer.tsx b/packages/react/src/SchemaRenderer.tsx index 077815d5d1..73ac27a2ff 100644 --- a/packages/react/src/SchemaRenderer.tsx +++ b/packages/react/src/SchemaRenderer.tsx @@ -19,6 +19,7 @@ import { debugTimeEnd, DebugCollector, validateSchema, + hasDeclaredPredicate, hasResponsiveStyles, scopeClassFor, compileScopedStyles, @@ -319,11 +320,41 @@ export const SchemaRenderer = forwardRef { - if (newSchema.disabled !== undefined) { + if (hasDeclaredPredicate(newSchema.disabled)) { return evaluator.evaluateCondition(newSchema.disabled); } - if (newSchema.disabledOn !== undefined) { + if (hasDeclaredPredicate(newSchema.disabledOn)) { return evaluator.evaluateCondition(newSchema.disabledOn); } return false; diff --git a/packages/react/src/__tests__/SchemaRenderer.disabledDeclaredGate.test.tsx b/packages/react/src/__tests__/SchemaRenderer.disabledDeclaredGate.test.tsx new file mode 100644 index 0000000000..26ae2ec4b9 --- /dev/null +++ b/packages/react/src/__tests__/SchemaRenderer.disabledDeclaredGate.test.tsx @@ -0,0 +1,244 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#3862 — `SchemaRenderer`'s inline `disabled` gate asked + * `newSchema.disabled !== undefined`, the WIDEST of the three spellings this + * repo had for "is a gate declared?", on the one key where width is not free. + * + * `evaluateCondition` answers an empty predicate with `true` — "no condition, so + * visible/enabled". On `visible` that `true` is negated and means SHOW, which is + * what "no gate" means anyway, so the `visible` chain above is benign. On + * `disabled` it means GREY. So `disabled: ''` set `_disabled = true`, and one + * notch wider than the `!= null` family, so did `disabled: null`. + * + * Two things make this the generic-path defect rather than one renderer's: + * + * • the block runs in the `evaluatedSchema` useMemo with no type branch, so it + * covers EVERY node that renders through `SchemaRenderer`. + * • `_disabled` is not an internal marker. It is stripped from + * `componentProps` and re-injected as `disabled: __disabled || undefined`, + * so any component taking a `disabled` prop — inputs, buttons, form fields — + * is really disabled. That forwarding is pinned below, because "the flag is + * not set" and "the prop does not arrive" are different claims and only the + * second is what a user sees. + * + * The fix reads core's one definition (`hasDeclaredPredicate`, objectui#3850's + * ruling) instead of adding a fourth local spelling — the `&& !== ''` this card + * explicitly ruled out. + * + * ## What each case detects + * + * • `disabled: ''` / `null` / `' '` / `{ dialect, source: '' }` / + * `{ source: '' }` → NOT disabled, and no `disabled` prop forwarded. THE + * defect. `''` and `null` are the two rows the card measured; the envelope is + * objectui#3850's shape reaching this path too. + * • `disabled: true` / a truthy expression / a truthy CEL envelope → still + * disabled. Anti-mutation guards: "never disable" satisfies most of this file + * alone, and these refuse it. + * • `disabled: false` → still not disabled, and `disabled: 0` → not disabled + * (junk fails open, as it now does at every other gate). + * • `disabledOn` in each of those directions → the alias reads the same + * definition, not a copy of it. + * • precedence: an EMPTY `disabled` no longer short-circuits the chain, so a + * declared `disabledOn` is finally consulted. This case is the one that would + * stay green if someone "fixed" the defect by deleting the `disabled` leg. + * • the `visible` chain, unchanged: it keeps `!== undefined` (its alias + * precedence is load-bearing and its `true` is benign), so `visible: ''` + * still renders and `visible: false` still hides. + * • the `hidden` / `hiddenOn` legs of that same chain, which are NOT negated + * and therefore have this defect with the polarity that makes the node + * VANISH. Measured while writing the equivalence cases, out of this ruling's + * scope, filed as objectui#3955 and pinned here as a documented divergence + * rather than left for the next reader to rediscover. + * + * ## Reverse verification (direction predicted before running) + * + * Restoring `newSchema.disabled !== undefined` / `disabledOn !== undefined` must + * turn RED exactly the five empty-shape cases on each key (`disabled`, + * `disabledOn`), their forwarding cases, the `0` junk case and the precedence + * case — and leave every `true` / `false` / expression / `visible` case GREEN. + * Nothing here can go red in the other direction: the change only ever removes a + * `disabled` prop, never adds one. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; +import { ComponentRegistry } from '@object-ui/core'; +import { SchemaRenderer } from '../SchemaRenderer'; +import { SchemaRendererContext } from '../context/SchemaRendererContext'; + +/** + * Records the `disabled` prop EXACTLY as it arrives — `absent` when the renderer + * forwarded nothing, so the pin can tell "no prop" from "disabled={false}". + */ +const Probe = (props: { disabled?: unknown }) => ( +
+); + +const DATA = { status: 'locked', readOnly: true, unlocked: false }; + +function renderNode(schema: Record) { + return render( + + + , + ); +} + +/** The forwarded prop, or `null` when the node did not render at all. */ +function disabledProp(): string | null { + const el = screen.queryByTestId('probe'); + return el ? el.getAttribute('data-disabled-prop') : null; +} + +const EMPTY_SHAPES: Array<{ label: string; value: unknown }> = [ + { label: "'' (empty string)", value: '' }, + { label: 'null', value: null }, + { label: "' ' (whitespace only)", value: ' ' }, + { label: "{ dialect: 'cel', source: '' } (what `objectstack build` emits)", value: { dialect: 'cel', source: '' } }, + { label: "{ source: '' } (envelope without a dialect)", value: { source: '' } }, +]; + +describe('SchemaRenderer `disabled` — an empty predicate is not a declared gate (objectui#3862)', () => { + beforeEach(() => { + ComponentRegistry.register('probe-3862', Probe as never); + }); + afterEach(() => { + ComponentRegistry.unregister?.('probe-3862'); + }); + + it.each(EMPTY_SHAPES)('disabled: $label → not disabled, and no `disabled` prop forwarded', ({ value }) => { + renderNode({ disabled: value }); + // Both halves of the claim: the node renders, and the prop channel + // (`disabled: __disabled || undefined`) carries nothing. + expect(disabledProp()).toBe('absent'); + }); + + it.each(EMPTY_SHAPES)('disabledOn: $label → not disabled either (the alias reads the same definition)', ({ value }) => { + renderNode({ disabledOn: value }); + expect(disabledProp()).toBe('absent'); + }); + + it('disabled: 0 (not a predicate) → not disabled — junk fails open here too', () => { + renderNode({ disabled: 0 }); + expect(disabledProp()).toBe('absent'); + }); + + it('disabled: true → still disabled, and the prop is forwarded', () => { + renderNode({ disabled: true }); + expect(disabledProp()).toBe('true'); + }); + + it('disabled: false → still not disabled', () => { + renderNode({ disabled: false }); + expect(disabledProp()).toBe('absent'); + }); + + it('no `disabled` at all → not disabled', () => { + renderNode({}); + expect(disabledProp()).toBe('absent'); + }); + + it('an expression-valued `disabled` keeps its verdict, both ways', () => { + const { unmount } = renderNode({ disabled: '${data.status === "locked"}' }); + expect(disabledProp()).toBe('true'); + unmount(); + renderNode({ disabled: '${data.unlocked}' }); + expect(disabledProp()).toBe('absent'); + }); + + it("a non-empty CEL envelope keeps its verdict, both ways", () => { + const { unmount } = renderNode({ disabled: { dialect: 'cel', source: 'true' } }); + expect(disabledProp()).toBe('true'); + unmount(); + renderNode({ disabled: { dialect: 'cel', source: 'false' } }); + expect(disabledProp()).toBe('absent'); + }); + + it('an expression-valued `disabledOn` keeps its verdict', () => { + renderNode({ disabledOn: '${data.readOnly}' }); + expect(disabledProp()).toBe('true'); + }); +}); + +describe('SchemaRenderer `disabled` chain precedence (objectui#3862)', () => { + beforeEach(() => { + ComponentRegistry.register('probe-3862', Probe as never); + }); + afterEach(() => { + ComponentRegistry.unregister?.('probe-3862'); + }); + + it('an EMPTY `disabled` no longer short-circuits — a declared `disabledOn` is consulted', () => { + // Before: `'' !== undefined` won the chain, `evaluateCondition('')` was + // `true`, and the node was disabled for a reason no key stated. Now the + // empty leg is not a gate, so the declared alias decides. + renderNode({ disabled: '', disabledOn: '${data.readOnly}' }); + expect(disabledProp()).toBe('true'); + }); + + it('… and the alias verdict is honoured in the other direction too', () => { + renderNode({ disabled: { dialect: 'cel', source: '' }, disabledOn: '${data.unlocked}' }); + expect(disabledProp()).toBe('absent'); + }); + + it('a DECLARED `disabled` still wins over `disabledOn`', () => { + renderNode({ disabled: false, disabledOn: true }); + expect(disabledProp()).toBe('absent'); + }); +}); + +describe('SchemaRenderer `visible` chain is untouched by this change (objectui#3862)', () => { + beforeEach(() => { + ComponentRegistry.register('probe-3862', Probe as never); + }); + afterEach(() => { + ComponentRegistry.unregister?.('probe-3862'); + }); + + it.each(EMPTY_SHAPES)('visible: $label → still rendered', ({ value }) => { + renderNode({ visible: value }); + expect(screen.getByTestId('probe')).toBeInTheDocument(); + }); + + it('visible: false → still hidden; visible: true → still rendered', () => { + const { unmount } = renderNode({ visible: false }); + expect(disabledProp()).toBeNull(); + unmount(); + renderNode({ visible: true }); + expect(screen.getByTestId('probe')).toBeInTheDocument(); + }); + + it('DOCUMENTED DIVERGENCE (objectui#3955): the `hidden` leg is NOT negated, so an empty predicate still hides', () => { + // The other polarity exit of the same asymmetry, in the same `useMemo`: + // `hidden` / `hiddenOn` return `evaluateCondition(...)` UN-negated, so + // "nothing to evaluate → true" means HIDE — the node vanishes for + // `hidden: ''` / `null` / `' '` / `{ dialect, source: '' }`. Measured, and + // deliberately out of this PR's ruling (objectui#3850's placement clause + // names the `disabled` / `disabledOn` legs; the `visible` family keeps + // `!== undefined` and its alias precedence). Pinned as the current state so + // the next reader sees it is known, with the issue that owns it — this + // expectation is what goes RED when objectui#3955 is fixed, which is the + // signal to move these rows into the fixed column. + renderNode({ hidden: '' }); + expect(screen.queryByTestId('probe')).toBeNull(); + const { unmount } = renderNode({ hidden: { dialect: 'cel', source: '' } }); + expect(screen.queryByTestId('probe')).toBeNull(); + unmount(); + // The `visible` legs of the same chain ARE negated, which is why the same + // empty value is benign there — the cases above this one. + renderNode({ visible: '' }); + expect(screen.getByTestId('probe')).toBeInTheDocument(); + }); +});