diff --git a/.changeset/predicate-scope-converge-3955-3957-3960.md b/.changeset/predicate-scope-converge-3955-3957-3960.md new file mode 100644 index 000000000..09c22ccc7 --- /dev/null +++ b/.changeset/predicate-scope-converge-3955-3957-3960.md @@ -0,0 +1,73 @@ +--- +"@object-ui/core": patch +"@object-ui/react": patch +--- + +Blank predicates and non-predicate values are no longer gates, at the last three entries that still judged them (objectui#3955, objectui#3957, objectui#3960) + +objectui#3850 sank "is a predicate gate DECLARED here?" into one definition, +`@object-ui/core`'s `hasDeclaredPredicate`. Three places were left out of that +ruling's placement clause, each with the same shape of defect: the evaluator's +single default for "there is nothing here to evaluate" is `true`, meaning +*visible/enabled*, and wherever a too-wide "declared" test hands it an empty +predicate on an INVERTED key, that `true` turns a control off for a value the +metadata never used to say anything. + +**`SchemaRenderer`'s `hidden` / `hiddenOn` legs (objectui#3955)** asked +`!== undefined` and did NOT negate the verdict, so an empty predicate meant HIDE +and the node disappeared — on the generic rendering path, since that block runs +for every schema type. Harder to diagnose than the `disabled` twin objectui#3862 +fixed: a greyed-out control is still on screen, while a node that never rendered +is indistinguishable from metadata that meant to hide it. Both legs now read the +shared definition. + +**The "blank" criterion now covers the envelope spelling (objectui#3960).** The +definition trimmed a whitespace-only STRING and not an envelope's whitespace-only +`source`, because `toPredicateInput` folds a `source` of `''` and does not trim. +So `{ dialect: 'cel', source: ' ' }` was a declared gate whose verdict came from +core's own CEL entry calling that exact value "no predicate" (`if (!source.trim()) +return true`) — `disabled` greyed out forever and `ActionRunner.execute` answered +`{ success: false, error: 'Action is disabled' }` with the handler never invoked. +Blankness is now decided once for both spellings, at the definition. The +NORMALIZER's contract is deliberately unchanged: "what shape does the evaluator +accept" is not the same question as "is there a condition", and moving the trim +there would have flipped verdicts for every +`useCondition(toPredicateInput(…))` call site, including container-level `visible` +reads that never asked this question at all. + +**`ActionEngine.getActionsForLocation`'s `visible` filter (objectui#3957)** was the +last consumer answering the question with a range of its own — three empty +spellings folded by hand, everything else coerced with `Boolean(raw)`. It now reads +the shared definition and the coercion branch is gone, so one value no longer gets +two answers depending on whether an action was surfaced by the engine or rendered +standalone (the invariant objectui#3314 established). Its fail-CLOSED posture on a +predicate that THROWS is untouched (`throwOnError: true` + `warnHiddenPredicate`): +"the predicate faulted" and "there is no predicate" are different facts. + +Behaviour changes, before → after. Observation-class: each needs an author to write +an empty/blank predicate or a non-predicate value, and there is no known user path +today. + +| value | `ActionEngine` `visible` | `SchemaRenderer` `hidden` | `disabled` (action face + generic path) | `ActionRunner.execute` `disabled` | +|---|---|---|---|---| +| `''` / `null` | shown → shown | HIDDEN → rendered | unchanged | unchanged | +| `' '` (blank text) | HIDDEN → shown | HIDDEN → rendered | unchanged | unchanged | +| `0` / `NaN` | HIDDEN → shown | HIDDEN → rendered | unchanged | unchanged | +| `{}` / `[]` | shown → shown | HIDDEN → rendered | unchanged | unchanged | +| `{ dialect: 'cel', source: '' }` | shown → shown | HIDDEN → rendered | unchanged | unchanged | +| `{ dialect: 'cel', source: ' ' }` | shown → shown | HIDDEN → rendered | GREY → on | refused → runs | +| `{ source: ' ' }` (no dialect) | HIDDEN → shown | HIDDEN → rendered | GREY → on | refused → runs | +| `true` / `false` / bare CEL / `${…}` / a non-blank 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 — a declared `false` is still a verdict, not a +missing gate (objectui#3812), and blankness is `trim()`, not "short": `{ dialect: +'cel', source: ' x ' }` is a predicate. One alias precedence changes with the +`hidden` legs and is pinned rather than claimed as an equivalence: an undeclared +`hidden` no longer short-circuits the chain, so a declared `hiddenOn` is finally +consulted. + +`SchemaRenderer`'s four `visible*` legs keep `!== undefined` deliberately, as +objectui#3850 ruled: their `true` is negated, so an empty predicate already lands +on "shown", and narrowing them would change alias precedence rather than fix +anything. diff --git a/packages/core/src/actions/ActionEngine.ts b/packages/core/src/actions/ActionEngine.ts index cc4875919..040acb217 100644 --- a/packages/core/src/actions/ActionEngine.ts +++ b/packages/core/src/actions/ActionEngine.ts @@ -19,6 +19,7 @@ import { ActionRunner, type ActionDef, type ActionContext, type ActionResult } from './ActionRunner'; import { toPredicateInput } from '../evaluator/predicateInput.js'; +import { hasDeclaredPredicate } from '../evaluator/declaredPredicate.js'; /** * Action location types — re-exported from `@objectstack/spec/ui` so the @@ -187,13 +188,18 @@ export class ActionEngine { * Filtering applied (in order): * 1. `locations.includes(location)` — location/region match * 2. `action.visible` — evaluated against the runner's current context - * ({ record, recordId, objectName, user, … }). Missing or `true` - * passes; any other value is coerced to boolean. Evaluator errors - * hide the action (fail-closed) rather than throwing — this matches - * the contract used by every individual action renderer - * (`action-button`, `action-menu`, `action-bar`, …) so the same - * action behaves identically whether surfaced via the engine or - * consumed standalone. + * ({ record, recordId, objectName, user, … }) whenever a gate is + * DECLARED, which is asked through core's one definition + * `hasDeclaredPredicate` (objectui#3850's ruling, adopted here by + * objectui#3957). No gate — absent, `''`, blank predicate text in either + * spelling, an envelope with no evaluable `source`, or a value that is + * not a predicate at all — means visible. Evaluator errors hide the + * action (fail-closed) rather than throwing — this matches the contract + * used by every individual action renderer (`action-button`, + * `action-menu`, `action-bar`, …) so the same action behaves identically + * whether surfaced via the engine or consumed standalone, which is now + * true of the gate in front of the verdict as well as the verdict + * (objectui#3314: one value, one answer, whichever entry reads it). * * Predicate normalization goes through the shared `toPredicateInput` * (`@object-ui/core`'s canonical helper — semantically identical to @@ -227,20 +233,35 @@ export class ActionEngine { }) .filter(ra => { const raw = ra.action.visible; - if (raw == null || raw === '' || raw === true) return true; - if (raw === false) return false; + // Ask "is a `visible` gate DECLARED?" through the repo's ONE definition + // (`evaluator/declaredPredicate.ts`, objectui#3850's ruling), which is + // what the renderers, `SchemaRenderer` and `ActionRunner`'s execution + // gates ask. This filter used to answer it with a range of its own — + // three empty spellings folded by hand (`null` / `''` / an envelope with + // an empty `source`) and everything else coerced with `Boolean(raw)` — + // and it was the LAST consumer to do so, so the same value got two + // answers depending on which entry read it: `visible: 0` / `NaN` hid the + // action here and showed it on the renderer face, and `visible: ' '` + // hid it here because the normalizer wraps a blank string into `'${ }'` + // whose verdict is falsy. That is the shape objectui#3314's invariant + // forbids, and it is fixed by deleting a range, not by adding one + // (objectui#3957). A value that is not a predicate, or a predicate that + // says nothing, must not be the reason an action is hidden from + // everyone — the same fail-open posture `ActionRunner` already committed + // to for `disabled` (`catch { isDisabled = false }`). + // + // Booleans need no branch of their own: they are DECLARED (`visible: + // false` is a verdict, objectui#3812) and `evaluateCondition` + // short-circuits them. + if (!hasDeclaredPredicate(raw)) return true; // #3314 — one shared normalization, not two. Hand-rolling the envelope // unwrap here is what dropped `dialect: 'cel'` and demoted the // predicate to the legacy JS engine; `toPredicateInput` keeps the // envelope so `evaluateCondition` can route it to `@objectstack/formula` // (the engine the server enforces with), exactly as the renderers do. + // Declared ⇒ the normalizer left something to evaluate, so there is no + // `undefined` case left to handle here. const expr = toPredicateInput(raw); - if (expr === undefined) { - // Not an evaluable predicate: an envelope with an empty `source` - // (→ nothing declared → visible), or a stray non-predicate value, - // which keeps the historical `Boolean(raw)` coercion. - return typeof raw === 'object' ? true : Boolean(raw); - } try { // `throwOnError: true` is required for fail-closed semantics: the // evaluator's default behavior swallows expression errors and diff --git a/packages/core/src/actions/ActionRunner.ts b/packages/core/src/actions/ActionRunner.ts index 3724cbb73..719f65113 100644 --- a/packages/core/src/actions/ActionRunner.ts +++ b/packages/core/src/actions/ActionRunner.ts @@ -593,13 +593,14 @@ function withIdentityAlias(context: ActionContext): ActionContext { * 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` - * (`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 - * and in the shared definition. + * The `visible`-filter template in `ActionEngine.getActionsForLocation` used to + * be the one place this scope was NOT shared: its non-predicate branch kept a + * historical `Boolean(raw)` coercion, so `0` there meant "hidden" — fail-CLOSED + * on junk, the opposite of what this module committed to for `disabled` + * (`catch { isDisabled = false }`). That filter now reads + * {@link hasDeclaredPredicate} too (objectui#3957), so a value that is not a + * predicate at all decides nothing on either key, at either entry: `0` / `{}` are + * "no gate" here, in the engine filter, and in the shared definition. */ export class ActionRunner { diff --git a/packages/core/src/actions/__tests__/ActionEngine.visibility.test.ts b/packages/core/src/actions/__tests__/ActionEngine.visibility.test.ts index c91ecba68..699612c24 100644 --- a/packages/core/src/actions/__tests__/ActionEngine.visibility.test.ts +++ b/packages/core/src/actions/__tests__/ActionEngine.visibility.test.ts @@ -21,10 +21,18 @@ * 4. Predicate errors fail closed (action hidden), not open. * 5. `null`/`undefined`/`''`/`true` all mean "always visible". * 6. Literal `false` always hides. + * + * Since objectui#3957 the "is a gate declared?" half of that contract is not this + * filter's own any more: it reads core's one definition `hasDeclaredPredicate` + * (`evaluator/declaredPredicate.ts`, objectui#3850's ruling), the same one the + * action renderers, `SchemaRenderer` and `ActionRunner`'s execution gates ask. The + * suite at the bottom of this file is that convergence; rules 1-6 above are + * unchanged by it. */ import { describe, it, expect, beforeEach } from 'vitest'; import { ActionEngine } from '../ActionEngine'; +import { hasDeclaredPredicate } from '../../evaluator/declaredPredicate'; import type { ActionDef } from '../ActionRunner'; function makeEngine(context: any) { @@ -365,3 +373,125 @@ describe('ActionEngine.getActionsForLocation — visibility filter', () => { }); }); + +/** + * objectui#3957 — the engine filter was the LAST consumer answering "is a + * `visible` gate declared?" with a range of its own. + * + * It folded three empty spellings by hand (`raw == null || raw === '' || raw === + * true`), passed an envelope with an empty `source` through + * `toPredicateInput`'s fold, and coerced everything else with `Boolean(raw)`. Two + * classes of value therefore got a different answer here than at every other + * entry: + * + * value | engine (before) | renderer face | shared definition + * 0 / NaN | HIDDEN | shown | no gate → shown + * ' ' (blank) | HIDDEN | shown | no gate → shown + * {} / '' | shown | shown | no gate → shown + * {cel, source:''}| shown | shown | no gate → shown + * + * `' '` never even reached the `Boolean(raw)` branch: the normalizer wraps a + * blank string into `'${ }'`, which is not `undefined`, so it was EVALUATED and + * came out falsy. One value, two answers, which is the shape objectui#3314's + * invariant forbids — and it is fixed by deleting a range, not adding one: the + * filter now asks `hasDeclaredPredicate` and the `Boolean(raw)` branch is gone. + * + * Cross-face parity for the same values lives in + * `packages/react/src/hooks/__tests__/actionPredicate.parity.test.tsx`, which can + * see both faces; core cannot import the renderer packages. + * + * ## Reverse verification (direction predicted before running) + * + * Restoring the hand-rolled range (the three folded spellings + `return typeof raw + * === 'object' ? true : Boolean(raw)`) must turn RED exactly the rows this suite + * calls "no gate" that the old range answered differently — `0`, `NaN`, `' '`, + * `'\t\n'`, and the blank-`source` envelopes — and leave GREEN every row the two + * ranges agreed on (`''`, `null`, `undefined`, `{}`, `[]`, the empty-`source` + * envelope) plus every anti-mutation row below. Nothing can go red in the other + * direction: this change only ever stops hiding an action. + */ +describe('ActionEngine `visible` reads the ONE declared-gate definition (objectui#3957)', () => { + const CTX = { record: { id: 'r1', status: 'open' }, user: { id: 'u1' } }; + + /** Does the filter surface an action carrying this `visible` value? */ + function shows(visible: unknown): boolean { + const engine = new ActionEngine({ ...CTX }); + engine.registerAction( + { name: 'probe', type: 'api', visible } as unknown as ActionDef, + { locations: ['record_section'] }, + ); + return engine.getActionsForLocation('record_section').length === 1; + } + + /** + * Every shape the shared definition calls "nothing to evaluate". `changed` + * marks the ones the engine's own range answered differently — the rows + * objectui#3957 measured as divergent from the renderer face. + */ + const NO_GATE: Array<{ label: string; value: unknown; changed: boolean }> = [ + { label: 'undefined (no key)', value: undefined, changed: false }, + { label: 'null', value: null, changed: false }, + { label: "'' (empty predicate)", value: '', changed: false }, + { label: "' ' (blank predicate text)", value: ' ', changed: true }, + { label: "'\\t\\n' (other blanks)", value: '\t\n', changed: true }, + { label: '0 (not a predicate)', value: 0, changed: true }, + { label: 'NaN (not a predicate)', value: NaN, changed: true }, + { label: '{} (no source)', value: {}, changed: false }, + { label: '[] (array)', value: [], changed: false }, + { label: "{ dialect: 'cel' } (no source key)", value: { dialect: 'cel' }, changed: false }, + { label: "{ dialect: 'cel', source: '' } (what `objectstack build` emits)", value: { dialect: 'cel', source: '' }, changed: false }, + { label: "{ dialect: 'cel', source: ' ' } (blank source — objectui#3960)", value: { dialect: 'cel', source: ' ' }, changed: false }, + { label: "{ source: ' ' } (blank source, no dialect)", value: { source: ' ' }, changed: true }, + ]; + + it.each(NO_GATE)('visible: $label → no gate, so the action is surfaced', ({ value }) => { + // Both halves of the claim: the shared definition says "not declared", and + // this filter agrees. Asserting the definition here is what ties the row set + // to `hasDeclaredPredicate` instead of to a list that could drift from it. + expect(hasDeclaredPredicate(value)).toBe(false); + expect(shows(value)).toBe(true); + }); + + it('the rows whose verdict CHANGED are exactly the ones the engine used to answer alone', () => { + // Guards the report's per-value table; a later edit that widens the blast + // radius fails here rather than passing quietly. + expect(NO_GATE.filter(s => s.changed).map(s => s.label)).toEqual([ + "' ' (blank predicate text)", + "'\\t\\n' (other blanks)", + '0 (not a predicate)', + 'NaN (not a predicate)', + "{ source: ' ' } (blank source, no dialect)", + ]); + }); + + it('a DECLARED gate is still evaluated in both directions (anti-mutation)', () => { + // "Surface everything" satisfies every case above. These refuse it. + expect(shows(true)).toBe(true); + expect(shows(false)).toBe(false); + expect(shows('record.status == "open"')).toBe(true); + expect(shows('record.status == "closed"')).toBe(false); + expect(shows({ dialect: 'cel', source: 'record.status == "open"' })).toBe(true); + expect(shows({ dialect: 'cel', source: 'record.status == "closed"' })).toBe(false); + // A blank source is "no gate", but one significant character is a predicate: + expect(shows({ dialect: 'cel', source: ' record.status == "closed" ' })).toBe(false); + }); + + it('a THROWING predicate still fails closed and still warns (the posture is untouched)', () => { + // The one place `visible` is deliberately fail-CLOSED, and the reason this + // change is not "the engine went fail-open": a predicate that FAULTED said + // something the evaluator could not answer, which is a different fact from a + // value that declares nothing. `throwOnError` + `warnHiddenPredicate` stay. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const engine = new ActionEngine({ record: { id: 'r1' } }); + engine.registerAction( + { name: 'bare_field', type: 'api', visible: '!done' } as unknown as ActionDef, + { locations: ['record_section'] }, + ); + expect(engine.getActionsForLocation('record_section')).toHaveLength(0); + expect(warn.mock.calls.filter(c => String(c[0]).includes('bare_field'))).toHaveLength(1); + } finally { + warn.mockRestore(); + } + }); +}); diff --git a/packages/core/src/actions/__tests__/ActionRunner.conditionGate.test.ts b/packages/core/src/actions/__tests__/ActionRunner.conditionGate.test.ts index aefc0707f..f2acb1ef1 100644 --- a/packages/core/src/actions/__tests__/ActionRunner.conditionGate.test.ts +++ b/packages/core/src/actions/__tests__/ActionRunner.conditionGate.test.ts @@ -47,9 +47,16 @@ * (string identity, `trim()`, envelope `source`), so each is its own * mutation detector. * • the non-predicate junk rows (`0`, `{}`) → still run, and NOT because the - * old truthiness test skipped them. See the divergence test below: this is - * a deliberate departure from `ActionEngine.getActionsForLocation`'s - * `Boolean(raw)` junk branch, which would have `0` block here. + * old truthiness test skipped them. This used to be a deliberate departure + * from `ActionEngine.getActionsForLocation`'s `Boolean(raw)` junk branch, + * which would have `0` block here; objectui#3957 moved that filter onto the + * same shared definition, so the case below is now a CONVERGENCE pin rather + * than a documented divergence. + * • `condition: { dialect: 'cel', source: ' ' }` → still runs, and after + * objectui#3960 for the sound reason rather than the accidental one: it was + * declared-and-`true` (a blank CEL source evaluates to "no condition → + * `true`"), which on THIS key happens to mean "execute" — the same value on + * `disabled` was blocking execution. Nothing declared now, same verdict. * * ## Scope: what this change does NOT touch * @@ -87,6 +94,7 @@ import { ActionRunner, type ActionContext, type ActionDef } from '../ActionRunne import { ActionEngine } from '../ActionEngine'; import { ExpressionEvaluator } from '../../evaluator/ExpressionEvaluator'; import { toPredicateInput } from '../../evaluator/predicateInput'; +import { hasDeclaredPredicate } from '../../evaluator/declaredPredicate'; const CONTEXT: ActionContext = { data: { id: 1 }, @@ -155,6 +163,17 @@ const SHAPES: Shape[] = [ condition: { dialect: 'cel', source: '' }, blocked: false, }, + { + // objectui#3960's fourth empty spelling. Unchanged VERDICT on this key and a + // changed reason: it was a declared gate whose blank CEL source evaluated to + // `true` ("no condition → visible/enabled"), which on `condition` means + // execute; now nothing is declared. The same value on `disabled` was blocking + // execution, which is why the fix belongs to the shared definition and not to + // either gate — see `ActionRunner.disabledGate.test.ts`. + label: "condition: { dialect: 'cel', source: ' ' } (blank source — objectui#3960)", + condition: { dialect: 'cel', source: ' ' }, + blocked: false, + }, // ── unchanged: non-predicate junk fails open ──────────────────────────── { label: 'condition: 0 (not a predicate)', condition: 0, blocked: false }, { label: 'condition: {} (not a predicate)', condition: {}, blocked: false }, @@ -245,17 +264,20 @@ describe('ActionRunner.execute — declared `condition` gate (objectui#3872)', ( describe('why the `condition` gate cannot ask truthiness (objectui#3872)', () => { it('truthiness and declaredness disagree on exactly the declared booleans', () => { - // The mechanism in one table. `hasDeclaredPredicate` is module-private, so - // its two ingredients are exercised through their public spellings. - const declared = (v: unknown) => - typeof v === 'string' && v.trim() === '' ? false : toPredicateInput(v) !== undefined; + // The mechanism in one table, asked through the REAL definition. It used to + // be re-spelled inline here (`typeof v === 'string' && v.trim() === '' ? …`) + // because the helper was module-private; objectui#3850 sank it into + // `evaluator/declaredPredicate.ts`, and a copy of a definition that has moved + // is a twin that drifts — objectui#3960 widened the real one and the copy + // would have kept answering the old way. + const declared = hasDeclaredPredicate; // `false` is the divergence: not truthy, yet plainly declared. expect(Boolean(false)).toBe(false); expect(declared(false)).toBe(true); // Everywhere else the two questions agree, which is why one row changed. - for (const v of ['', ' ', 0, {}, { dialect: 'cel', source: '' }]) { + for (const v of ['', ' ', 0, {}, { dialect: 'cel', source: '' }, { dialect: 'cel', source: ' ' }]) { expect(declared(v), `${JSON.stringify(v)} declares no gate`).toBe(false); } for (const v of [true, 'user.role == "admin"', { dialect: 'cel', source: 'false' }]) { @@ -288,25 +310,34 @@ describe('why the `condition` gate cannot ask truthiness (objectui#3872)', () => expect(ev.evaluateCondition(toPredicateInput(truePredicate) as never)).toBe(true); }); - it('DOCUMENTED DIVERGENCE: the engine `visible` filter coerces junk, this gate does not', () => { - // `ActionEngine.getActionsForLocation` is the in-repo template this fix took - // its shape from, but its non-predicate branch keeps a historical - // `Boolean(raw)` coercion, so `visible: 0` HIDES. This gate deliberately - // 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. + it('CONVERGED (objectui#3957): the engine `visible` filter no longer coerces junk either', () => { + // This case used to be a DOCUMENTED DIVERGENCE. `ActionEngine. + // getActionsForLocation` is the in-repo template this gate took its shape + // from, but its non-predicate branch kept a historical `Boolean(raw)` + // coercion, so `visible: 0` HID an action the renderer face showed — one + // value, two answers, the shape objectui#3314's invariant forbids. This gate + // deliberately did not copy it (`catch { isDisabled = false }` had already + // committed this module to fail-OPEN on junk), and objectui#3850 landed + // without unifying the engine: its ruling covered the "declared?" definition + // and its placement, not that filter's own range. // - // 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. + // objectui#3957 moved the engine onto the same definition, so `0` is "no + // gate" at every entry. Kept here, converged, because the divergence is what + // this file documented — the assertion flipped from `toHaveLength(0)` to + // `toHaveLength(1)` and is now the pin that the two faces agree. const engine = new ActionEngine(CONTEXT); engine.registerAction( { name: 'junk_visible', type: 'script', target: '"ran"', visible: 0 } as unknown as ActionDef, { locations: ['record_section'] }, ); - expect(engine.getActionsForLocation('record_section')).toHaveLength(0); + expect(engine.getActionsForLocation('record_section').map(a => a.name)).toEqual(['junk_visible']); + // Anti-mutation: "the filter passes everything" satisfies the line above. A + // declared-and-false gate still hides, at the engine as at this gate. + const gated = new ActionEngine(CONTEXT); + gated.registerAction( + { name: 'off', type: 'script', target: '"ran"', visible: false } as unknown as ActionDef, + { locations: ['record_section'] }, + ); + expect(gated.getActionsForLocation('record_section')).toHaveLength(0); }); }); diff --git a/packages/core/src/actions/__tests__/ActionRunner.disabledGate.test.ts b/packages/core/src/actions/__tests__/ActionRunner.disabledGate.test.ts index 9ede7bec5..5f3a58d1b 100644 --- a/packages/core/src/actions/__tests__/ActionRunner.disabledGate.test.ts +++ b/packages/core/src/actions/__tests__/ActionRunner.disabledGate.test.ts @@ -24,6 +24,15 @@ * disabled absent | handler ran: true | {"success":true} * disabled false | handler ran: true | {"success":true} * + * …and two rows that stayed BLOCKED after this card's fix, until objectui#3960 + * widened the shared definition to blank predicate text in either spelling. + * Measured by running these two rows against `ab3ad4f3f`'s scope — the string-only + * `value.trim() === ''` half of the definition, which is what the reverse + * verification below restores: + * + * disabled {dialect:'cel',source:' '} | handler ran: false | {"success":false,"error":"Action is disabled"} + * disabled {source:' '} | handler ran: false | {"success":false,"error":"Action is disabled"} + * * So this was not "the click did nothing" — execution was refused and the caller * got an error naming a state the metadata never declared. * @@ -33,6 +42,14 @@ * the handler must run. THE defect; each is an independent mutation * detector, since the three arrive at "nothing to evaluate" by three * different routes (string identity, `trim()`, envelope `source`). + * • the FOURTH empty shape, added by objectui#3960 — an envelope whose `source` + * is blank but not empty, in both its dialect spellings. It was still BLOCKED + * after this card's own fix: the normalizer folds a `source` of `''` and does + * not trim, so the value reached `evaluateCondition`, which calls a blank + * source "no condition" and answers `true` — and on this key `true` means + * "Action is disabled". Same defect, same mechanism, blank moved inside the + * envelope; behaviour change, and the same fail-open direction as the rows + * above. * • `true` / a truthy expression / a truthy CEL envelope → still blocked. * Anti-mutation guards: "never block anything" satisfies most of this table * on its own, and these are what refuse it. @@ -85,11 +102,17 @@ * ## Reverse verification (direction predicted before running) * * Restoring the old gate (`action.disabled != null && action.disabled !== false`) - * while leaving evaluation untouched must turn exactly the five - * nothing-to-evaluate rows RED — `''`, `' '`, the empty envelope, `0`, `{}` — - * each naming its own shape, and leave `absent` / `false` / `true` / both - * expression rows / both non-empty envelope rows GREEN. That is the whole diff: - * this change can only stop blocking things, never start. + * while leaving evaluation untouched must turn exactly the seven + * nothing-to-evaluate rows RED — `''`, `' '`, the empty envelope, both + * blank-`source` envelopes, `0`, `{}` — each naming its own shape, and leave + * `absent` / `false` / `true` / both expression rows / both non-empty envelope + * rows GREEN. That is the whole diff: this change can only stop blocking things, + * never start. + * + * Reverting objectui#3960 alone (dropping the envelope half of the shared + * definition's `isBlankPredicateText`) must turn RED exactly the two + * blank-`source` rows and nothing else — including the parity test, whose column + * for those rows is a claim about the renderer face reading the SAME definition. */ import { describe, it, expect, vi } from 'vitest'; @@ -132,6 +155,30 @@ const SHAPES: Shape[] = [ blocked: false, rendererDisabled: false, }, + // ── behaviour change (objectui#3960): the FOURTH empty spelling ─────────── + // A `source` that is blank but not empty. Measured as BLOCKED on this gate + // before objectui#3960 — `toPredicateInput` folds a `source` of `''` and does + // not trim, so the envelope reached `evaluateCondition`, whose CEL entry + // answers `if (!source.trim()) return true`, and on this key that `true` means + // "Action is disabled". Declaredness now decides blankness in both spellings, + // so the handler runs. Parity is claimed on the same footing as the rows above: + // one definition, read by the renderer face too. + { + label: "disabled: { dialect: 'cel', source: ' ' } (blank source — objectui#3960)", + disabled: { dialect: 'cel', source: ' ' }, + blocked: false, + rendererDisabled: false, + }, + { + // The dialect-less spelling takes the other route through the normalizer + // (unwrapped and wrapped into `'${ }'`), so it is its own mutation + // detector — and its verdict came out the same way, `true` = blocked, via + // `evaluateCondition`'s `if (!trimmed) return true` instead of the CEL entry. + label: "disabled: { source: ' ' } (blank source, no dialect)", + disabled: { source: ' ' }, + blocked: false, + rendererDisabled: false, + }, // ── unchanged: ungated stays ungated ──────────────────────────────────── { label: 'disabled absent (undeclared)', absent: true, blocked: false, rendererDisabled: false }, { label: 'disabled: false (declared, verdict false)', disabled: false, blocked: false, rendererDisabled: false }, @@ -266,14 +313,17 @@ describe('why the gate cannot delegate "is there a condition?" to the verdict (o expect(ev.evaluateCondition({ dialect: 'cel', source: '' })).toBe(true); }); - it('toPredicateInput collapses two of the three empty shapes, and wraps the third', () => { - // Why the gate is not simply `toPredicateInput(x) !== undefined`: the - // whitespace string survives normalization as an evaluable-looking template, - // so the gate names it explicitly (same blank-source rule `evalRowPredicate` - // applies in `evaluator/listConditional.ts`). + it('toPredicateInput collapses the EMPTY shapes and passes the BLANK ones through', () => { + // Why the gate is not simply `toPredicateInput(x) !== undefined`: blank + // predicate text survives normalization as an evaluable-looking value in both + // its spellings — a wrapped template for the bare string, an intact envelope + // for a blank `source` — so the shared definition names blankness explicitly + // (objectui#3960; the same blank rule `evalRowPredicate` applies in + // `evaluator/listConditional.ts`). expect(toPredicateInput('')).toBeUndefined(); expect(toPredicateInput({ dialect: 'cel', source: '' })).toBeUndefined(); expect(toPredicateInput(' ')).toBe('${ }'); + expect(toPredicateInput({ dialect: 'cel', source: ' ' })).toEqual({ dialect: 'cel', source: ' ' }); }); it('normalizing a `${…}` predicate now agrees with the raw value (was objectui#3871)', () => { diff --git a/packages/core/src/evaluator/__tests__/declaredPredicate.test.ts b/packages/core/src/evaluator/__tests__/declaredPredicate.test.ts index 770713191..3b6786e79 100644 --- a/packages/core/src/evaluator/__tests__/declaredPredicate.test.ts +++ b/packages/core/src/evaluator/__tests__/declaredPredicate.test.ts @@ -23,30 +23,51 @@ * 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, + * • DERIVATION: for every shape except BLANK predicate text, * `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. + * • blank predicate TEXT is the single deliberate DIFFERENCE from the + * normalizer, in BOTH its spellings — the whitespace-only string (`' '`, + * which the normalizer wraps into `'${ }'`) and the whitespace-only + * envelope `source` (`{ dialect: 'cel', source: ' ' }`, which it passes + * through because it only folds a `source` of `''`). The disagreement list is + * asserted by NAME, so the difference reads as chosen rather than overlooked + * and cannot grow silently. + * + * ## objectui#3960 — the fourth empty spelling, and how it got here + * + * The blank-`source` envelope was found BY this suite: its first draft assumed + * the normalizer folds it, that assertion went red, and the shape turned out to + * be a fourth empty spelling outside objectui#3850's three-way enumeration. It + * was filed and pinned here as a documented residue — `hasDeclaredPredicate` + * answered "declared" while core's own CEL entry called the same value "no + * predicate", so `disabled` greyed out and `ActionRunner` refused to execute for + * a predicate that says nothing. objectui#3960 ruled it in: blankness is now + * decided for both spellings at once (`isBlankPredicateText`), and the residue + * case below has become the converged pin, one directory over from the mechanism + * it used to document. * * ## 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. + * Two mutations, two predicted directions: + * + * 1. narrowing the definition back to the renderer's historic scope + * (`value != null && value !== ''`) must turn RED exactly: the empty-envelope + * rows (both spellings), both blank-text rows, the blank-`source` envelope + * rows, 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. + * 2. reverting objectui#3960 alone (dropping the envelope half of + * `isBlankPredicateText`, i.e. `typeof value === 'string' && value.trim() === + * ''`) must turn RED exactly the three blank-`source` envelope rows, the + * four-spellings case, the converged case below, and the derivation case + * (whose named disagreement list shrinks by those three rows). Everything + * else stays GREEN — this half of the change only ever moves a shape from + * "declared" to "not declared". */ import { describe, it, expect } from 'vitest'; @@ -64,10 +85,15 @@ const SHAPES: Array<{ label: string; value: unknown; declared: boolean }> = [ { 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. + // objectui#3960 — blank predicate text in its ENVELOPE spelling. These three + // rows were the documented residue of objectui#3850's three-way enumeration + // (the normalizer folds a `source` of `''` and does not trim), and each takes a + // different route: the `cel` envelope survives normalization intact, the + // dialect-less one is unwrapped and wrapped into `'${ }'`, and `'\n'` proves + // the rule is `trim()` and not a literal-space comparison. + { label: "{ dialect: 'cel', source: ' ' } (blank source — objectui#3960)", value: { dialect: 'cel', source: ' ' }, declared: false }, + { label: "{ source: ' ' } (blank source, no dialect)", value: { source: ' ' }, declared: false }, + { label: "{ dialect: 'cel', source: '\\n' } (other blanks)", value: { dialect: 'cel', source: '\n' }, declared: false }, // ── not a predicate at all → fail open, never a reason to disable ────── { label: '0', value: 0, declared: false }, { label: '{} (no source)', value: {}, declared: false }, @@ -87,53 +113,72 @@ describe('hasDeclaredPredicate — the scope objectui#3850 ruled on', () => { 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. + it('the four empty spellings are one answer, not four', () => { + // `''` was objectui#3842's half, the empty envelope objectui#3850's, the + // whitespace string objectui#3848's, the blank-`source` envelope + // objectui#3960's. One definition, so they cannot diverge again. expect([ hasDeclaredPredicate(''), hasDeclaredPredicate(' '), hasDeclaredPredicate({ dialect: 'cel', source: '' }), - ]).toEqual([false, false, false]); + hasDeclaredPredicate({ dialect: 'cel', source: ' ' }), + ]).toEqual([false, 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', () => { + it('agrees with `toPredicateInput(x) !== undefined` on every shape but BLANK predicate text', () => { const disagreements = SHAPES.filter( s => hasDeclaredPredicate(s.value) !== (toPredicateInput(s.value) !== undefined), ).map(s => s.label); + // Named, not counted: the deliberate delta from the normalizer is exactly the + // two spellings of a blank predicate, and this list is what stops a third + // exception from being added without a reader noticing. expect(disagreements).toEqual([ "' ' (whitespace only)", "'\\t\\n' (other blanks)", + "{ dialect: 'cel', source: ' ' } (blank source — objectui#3960)", + "{ source: ' ' } (blank source, no dialect)", + "{ dialect: 'cel', source: '\\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. + it('blank predicate text is what the normalizer wraps or passes through instead of folding', () => { + // Which is why the definition names both spellings explicitly. The same blank + // rule already applies on the VALUE side — `evaluateCondition` + // (`if (!trimmed) return true`), `evaluateCelCondition` (`if (!source.trim())`) + // and `evalRowPredicate` (`listConditional.ts`). expect(toPredicateInput(' ')).toBe('${ }'); + expect(toPredicateInput({ dialect: 'cel', source: ' ' })).toEqual({ dialect: 'cel', source: ' ' }); + expect(toPredicateInput({ source: ' ' })).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 `''`: + it('objectui#3960 CONVERGED: a BLANK envelope `source` is not a declared gate', () => { + // The documented residue this suite left behind, turned around. The + // NORMALIZER's contract is deliberately unchanged — it still passes a blank + // `cel` source through, because "what shape does the evaluator accept" is not + // the same question as "is there a condition": 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": + // …and core's own CEL entry still 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. + // What changed is the half that was inconsistent — declaredness. A blank + // predicate is blank in both spellings now, so "declared gate + no condition + // → true" can no longer grey out a control (`disabled`), vanish a node + // (`hidden`, objectui#3955) or refuse an execution (`ActionRunner`) for a + // value the metadata never used to say anything. + expect(hasDeclaredPredicate({ dialect: 'cel', source: ' ' })).toBe(false); + expect(hasDeclaredPredicate({ source: ' ' })).toBe(false); + expect(hasDeclaredPredicate({ dialect: 'cel', source: '\n' })).toBe(false); + // Anti-mutation: "no envelope is ever declared" satisfies every line above. + // A non-blank source still declares a gate, in either dialect… + expect(hasDeclaredPredicate({ dialect: 'cel', source: 'false' })).toBe(true); + expect(hasDeclaredPredicate({ source: 'record.done' })).toBe(true); + // …and blankness is `trim()`, not "short": one significant character + // surrounded by whitespace is a predicate. + expect(hasDeclaredPredicate({ dialect: 'cel', source: ' x ' })).toBe(true); }); }); diff --git a/packages/core/src/evaluator/declaredPredicate.ts b/packages/core/src/evaluator/declaredPredicate.ts index db1698d11..e8871fe8c 100644 --- a/packages/core/src/evaluator/declaredPredicate.ts +++ b/packages/core/src/evaluator/declaredPredicate.ts @@ -8,6 +8,33 @@ import { toPredicateInput } from './predicateInput'; +/** + * Is the predicate TEXT blank — in EITHER spelling? (objectui#3960) + * + * `' '` and `{ dialect: 'cel', source: ' ' }` are the same author mistake + * written two ways, and {@link toPredicateInput} folds neither: it wraps a + * whitespace-only string into `'${ }'`, and it only folds an envelope whose + * `source` is exactly `''`. So blankness cannot be derived from the normalizer's + * answer and has to be stated once, here, for both spellings — trimming one and + * not the other is the asymmetry objectui#3960 measured (`disabled` greyed out, + * `ActionRunner` refused to execute, for a predicate that says nothing). + * + * This is not a fourth dialect of "empty": it is the SAME rule core's evaluation + * entries already apply on the value side — `evaluateCondition` + * (`if (!trimmed) return true`), `evaluateCelCondition` (`if (!source.trim()) + * return true`), `evalRowPredicate` (`listConditional.ts`) — brought to the one + * place that answers "is there a condition at all?", so the two halves cannot + * disagree about the same blank. + */ +function isBlankPredicateText(value: unknown): boolean { + if (typeof value === 'string') return value.trim() === ''; + if (value !== null && typeof value === 'object') { + const source = (value as { source?: unknown }).source; + if (typeof source === 'string') return source.trim() === ''; + } + return false; +} + /** * Is a predicate gate DECLARED on this value — i.e. after normalization, is * there still a CONDITION for {@link ExpressionEvaluator.evaluateCondition} to @@ -29,15 +56,20 @@ import { toPredicateInput } from './predicateInput'; * * ## What counts as "nothing to evaluate" * - * Exactly the shapes {@link toPredicateInput} folds to `undefined`, plus the one - * it wraps instead of folding: + * Exactly the shapes {@link toPredicateInput} folds to `undefined`, plus the two + * BLANK spellings it wraps or passes through 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` + * - blank predicate TEXT, in either spelling — a whitespace-only string + * (`' '`, which the normalizer wraps into `'${ }'`) and an envelope whose + * `source` is whitespace-only (`{ dialect: 'cel', source: ' ' }`, which the + * normalizer passes through because it only folds a `source` of `''`). + * Neither can be read off the normalizer's answer, so both are named here — + * see {@link isBlankPredicateText} for why this is the layer that says it. + * Core's other predicate entries already treat both as blank: + * `evaluateCondition` (`if (!trimmed) return true`), `evaluateCelCondition` + * (`if (!source.trim()) 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 @@ -54,18 +86,31 @@ import { toPredicateInput } from './predicateInput'; * 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) + * ## Why blankness is decided HERE and not in the normalizer (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. + * The blank-`source` envelope arrived one release late: objectui#3850's ruling + * enumerated three empty spellings, `toPredicateInput` folds a `source` of `''` + * and does not trim, so `{ dialect: 'cel', source: ' ' }` stayed "declared" + * while core's own CEL entry called that exact value "no predicate" — `disabled` + * greyed out and `ActionRunner` refused to execute for a predicate that says + * nothing (objectui#3960). The asymmetry was that this definition trimmed the + * STRING spelling of a blank predicate and not the ENVELOPE spelling of the same + * blank. + * + * Fixing it at the normalizer instead (`if (!src.trim()) return undefined`) would + * have aligned every `evaluateCondition(toPredicateInput(x))` consumer at once, + * and that is precisely why it is the wider, wrong lever: it changes what + * normalization MEANS, which is a shape question ("what does the evaluator + * accept"), to answer a declaredness question ("is there a condition"). Those are + * different concepts on purpose — a blank `source` is a shape the evaluator + * accepts and answers `true` for, by its own documented rule. The blast radius is + * the concrete difference: at the normalizer, a NON-cel blank envelope + * (`{ source: ' ' }` → `'${ }'` → falsy) flips verdict for every + * `useCondition(toPredicateInput(…))` call site including the container-level + * `visible` reads in `action-bar` / `action-group` / `action-menu` / + * `RelatedList` / `record-alert`, none of which ask this question at all; here it + * flips only for the consumers that DO ask it, and only on the inverted-polarity + * keys where "declared" is what turns a control off. * * ## Why the callers still evaluate the RAW value * @@ -91,8 +136,17 @@ import { toPredicateInput } from './predicateInput'; * renderer call sites are unchanged), `SchemaRenderer`'s `disabled` / * `disabledOn` chain reads it, and `ActionRunner`'s two gates read it instead of * a private twin. + * + * Two more consumers joined afterwards, and with them the last two places that + * answered this question with a range of their own: + * `SchemaRenderer`'s `hidden` / `hiddenOn` legs, whose verdict is NOT negated so + * an empty predicate made the node VANISH (objectui#3955), and + * `ActionEngine.getActionsForLocation`'s `visible` filter, which folded some + * empty spellings by hand and coerced the rest with `Boolean(raw)`, so + * `visible: 0` hid an action the renderers showed (objectui#3957). Nothing in the + * repo now asks "is a gate declared?" anywhere but here. */ export function hasDeclaredPredicate(value: unknown): boolean { - if (typeof value === 'string' && value.trim() === '') return false; + if (isBlankPredicateText(value)) return false; return toPredicateInput(value) !== undefined; } diff --git a/packages/react/src/SchemaRenderer.tsx b/packages/react/src/SchemaRenderer.tsx index 73ac27a2f..3529f94a9 100644 --- a/packages/react/src/SchemaRenderer.tsx +++ b/packages/react/src/SchemaRenderer.tsx @@ -306,10 +306,28 @@ export const SchemaRenderer = forwardRef = [ { label: "' ' (whitespace only)", value: ' ' }, { label: "{ dialect: 'cel', source: '' } (what `objectstack build` emits)", value: { dialect: 'cel', source: '' } }, { label: "{ source: '' } (envelope without a dialect)", value: { source: '' } }, + // objectui#3960 — the fourth empty spelling, arriving through the same shared + // definition. It was still a declared gate after this card's fix (the + // normalizer folds a `source` of `''` and does not trim), so the generic path + // greyed the control out for a predicate that says nothing; blankness is now + // decided for the envelope spelling too. + { label: "{ dialect: 'cel', source: ' ' } (blank source — objectui#3960)", value: { dialect: 'cel', source: ' ' } }, + { label: "{ source: ' ' } (blank source, no dialect)", value: { source: ' ' } }, ]; describe('SchemaRenderer `disabled` — an empty predicate is not a declared gate (objectui#3862)', () => { @@ -220,24 +234,28 @@ describe('SchemaRenderer `visible` chain is untouched by this change (objectui#3 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(); + it('CONVERGED (objectui#3955): the `hidden` leg reads the same definition, so an empty predicate no longer hides', () => { + // This case used to be a DOCUMENTED DIVERGENCE. The other polarity exit of the + // same asymmetry, in the same `useMemo`: `hidden` / `hiddenOn` return + // `evaluateCondition(...)` UN-negated, so "nothing to evaluate → true" meant + // HIDE and the node vanished for `hidden: ''` / `null` / `' '` / + // `{ dialect, source: '' }`. It was measured here while writing the + // equivalence cases and left out of objectui#3850's ruling on purpose (its + // placement clause named the `disabled` / `disabledOn` legs), then fixed as + // objectui#3955 — the assertions below flipped from "the node is gone" to + // "the node renders". + // + // The full table for both legs, including precedence and the anti-mutation + // guards, is in `SchemaRenderer.hiddenDeclaredGate.test.tsx`; these three + // lines stay here because this file is where the divergence was recorded. + const { unmount } = renderNode({ hidden: '' }); + expect(screen.getByTestId('probe')).toBeInTheDocument(); 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({ hidden: { dialect: 'cel', source: '' } }); + expect(screen.getByTestId('probe')).toBeInTheDocument(); + }); + + it('… while the `visible` legs are unchanged, which is why the same empty value was benign there', () => { renderNode({ visible: '' }); expect(screen.getByTestId('probe')).toBeInTheDocument(); }); diff --git a/packages/react/src/__tests__/SchemaRenderer.hiddenDeclaredGate.test.tsx b/packages/react/src/__tests__/SchemaRenderer.hiddenDeclaredGate.test.tsx new file mode 100644 index 000000000..7fe947e59 --- /dev/null +++ b/packages/react/src/__tests__/SchemaRenderer.hiddenDeclaredGate.test.tsx @@ -0,0 +1,251 @@ +/** + * 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#3955 — the REVERSE-POLARITY twin of objectui#3862, in the same + * `useMemo` of the same file. + * + * `SchemaRenderer`'s visibility chain has six legs. The four `visible*` ones are + * written `!evaluator.evaluateCondition(...)`; the `hidden` / `hiddenOn` pair is + * not negated. `evaluateCondition` documents exactly ONE default for "there is + * nothing here to evaluate": it returns `true`, meaning *visible/enabled*. Negated + * that default lands on "shown", which is what "no gate" means anyway — benign. + * Un-negated it means HIDE, so an empty predicate made the node disappear: + * + * value | visible* legs | hidden / hiddenOn (before) + * '' | rendered | HIDDEN + * ' ' (whitespace) | rendered | HIDDEN + * { dialect: 'cel', source: '' }| rendered | HIDDEN + * null | rendered | HIDDEN + * false | HIDDEN | rendered + * true | rendered | HIDDEN + * + * The `false` / `true` rows were already right — a declared verdict is honoured — + * and the four "empty" rows were the defect, with the widest possible spelling in + * front of them (`!== undefined`, so `hidden: null` counted too). + * + * Two things make this the generic-path defect rather than one renderer's, and + * make it HARDER to diagnose than its `disabled` twin: + * + * • the block runs in the `evaluatedSchema` useMemo with no type branch, so it + * covers EVERY node that renders through `SchemaRenderer`; + * • a greyed-out control is still on screen. A node that never rendered is + * indistinguishable from metadata that meant to hide it — the author sees + * "I wrote an empty `hidden` and the whole block vanished", with nothing on + * screen to attribute it to. + * + * `{ dialect, source: '' }` 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. + * + * The fix reads core's ONE definition (`hasDeclaredPredicate`, objectui#3850's + * ruling) rather than adding an Nth local `&& !== ''`, and it also brings the + * fourth empty spelling — a blank-but-not-empty envelope `source`, objectui#3960 — + * with it, since that widening happened in the same definition. + * + * ## What each case detects + * + * • the empty shapes on `hidden` and on `hiddenOn` → the node RENDERS. THE + * defect. Each shape reaches "nothing to evaluate" by its own route (string + * identity, `trim()`, envelope `source` empty, envelope `source` blank), so + * each is an independent mutation detector. + * • non-predicate junk (`0`, `{}`, `[]`) → renders. Fail-open on junk, the + * posture every other gate now takes. + * • `hidden: true` / a holding expression / a holding CEL envelope → STILL + * hidden. Anti-mutation guards: "never hide anything" satisfies most of this + * file on its own, and these refuse it. + * • `hidden: false` → still rendered, and the key is not forwarded as a DOM + * prop (it is stripped in the destructure below `evaluatedSchema`), so "not + * hidden" cannot be confused with "hidden={false} reached the component". + * • precedence, both directions: an UNDECLARED `hidden` no longer + * short-circuits, so a declared `hiddenOn` is finally consulted — and a + * DECLARED `hidden` still wins over `hiddenOn`, which is the case that would + * stay green if someone "fixed" the defect by deleting the `hidden` leg. + * • the `visible*` legs still come FIRST and still keep `!== undefined`: their + * alias precedence is load-bearing and their `true` is benign (objectui#3850's + * ruling fenced them off deliberately). + * + * ## Reverse verification (direction predicted before running) + * + * Restoring `newSchema.hidden !== undefined` / `hiddenOn !== undefined` must turn + * RED exactly: every empty-shape case on both keys, the junk cases, and the + * "undeclared `hidden` falls through to `hiddenOn`" precedence case (whose + * `hiddenOn: false` becomes unreachable). Every `true` / `false` / expression / + * envelope / `visible*` case stays GREEN — the change can only stop hiding a + * node, never start. + * + * Reverting the objectui#3960 half alone (envelope blankness) turns RED only the + * two blank-`source` rows on each key. Deleting either leg outright turns RED the + * "a DECLARED `hidden` wins" case, which no other case here would catch. + */ + +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 `hidden` prop exactly as it arrives, so "the node rendered" and + * "the schema key leaked into the DOM props" are separate observations. + */ +const Probe = (props: { hidden?: unknown }) => ( +
+); + +const DATA = { status: 'draft', archived: true, published: false }; + +function renderNode(schema: Record) { + return render( + + + , + ); +} + +/** Did the node render at all? */ +function rendered(): boolean { + return screen.queryByTestId('probe') !== null; +} + +const EMPTY_SHAPES: Array<{ label: string; value: unknown }> = [ + { label: "'' (empty predicate)", 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: '' } }, + // objectui#3960's fourth spelling, arriving through the same shared definition. + { label: "{ dialect: 'cel', source: ' ' } (blank source — objectui#3960)", value: { dialect: 'cel', source: ' ' } }, + { label: "{ source: ' ' } (blank source, no dialect)", value: { source: ' ' } }, +]; + +const JUNK_SHAPES: Array<{ label: string; value: unknown }> = [ + { label: '0', value: 0 }, + { label: '{} (no source)', value: {} }, + { label: '[] (array)', value: [] }, +]; + +describe('SchemaRenderer `hidden` — an empty predicate is not a declared gate (objectui#3955)', () => { + beforeEach(() => { + ComponentRegistry.register('probe-3955', Probe as never); + }); + afterEach(() => { + ComponentRegistry.unregister?.('probe-3955'); + }); + + it.each(EMPTY_SHAPES)('hidden: $label → the node renders', ({ value }) => { + renderNode({ hidden: value }); + expect(rendered()).toBe(true); + }); + + it.each(EMPTY_SHAPES)('hiddenOn: $label → the node renders too (the alias reads the same definition)', ({ value }) => { + renderNode({ hiddenOn: value }); + expect(rendered()).toBe(true); + }); + + it.each(JUNK_SHAPES)('hidden: $label (not a predicate) → the node renders — junk fails open', ({ value }) => { + renderNode({ hidden: value }); + expect(rendered()).toBe(true); + }); + + it('hidden: true → still hidden; hidden: false → still rendered, with no `hidden` prop forwarded', () => { + const { unmount } = renderNode({ hidden: true }); + expect(rendered()).toBe(false); + unmount(); + renderNode({ hidden: false }); + expect(rendered()).toBe(true); + // The schema key is metadata, not a DOM prop — it is stripped in the + // destructure, so a rendered node must not also carry `hidden={false}`. + expect(screen.getByTestId('probe')).toHaveAttribute('data-hidden-prop', 'absent'); + }); + + it('hiddenOn: true → still hidden; hiddenOn: false → still rendered', () => { + const { unmount } = renderNode({ hiddenOn: true }); + expect(rendered()).toBe(false); + unmount(); + renderNode({ hiddenOn: false }); + expect(rendered()).toBe(true); + }); + + it('an expression-valued `hidden` keeps its verdict, both ways', () => { + const { unmount } = renderNode({ hidden: '${data.status === "draft"}' }); + expect(rendered()).toBe(false); + unmount(); + renderNode({ hidden: '${data.published}' }); + expect(rendered()).toBe(true); + }); + + it('a non-empty CEL envelope keeps its verdict, both ways', () => { + const { unmount } = renderNode({ hidden: { dialect: 'cel', source: 'true' } }); + expect(rendered()).toBe(false); + unmount(); + renderNode({ hidden: { dialect: 'cel', source: 'false' } }); + expect(rendered()).toBe(true); + }); + + it('an expression-valued `hiddenOn` keeps its verdict', () => { + renderNode({ hiddenOn: '${data.archived}' }); + expect(rendered()).toBe(false); + }); + + it('a blank source is "no gate", but one significant character is a predicate', () => { + // Blankness is `trim()`, not "short": the same envelope with real text in it + // still hides, so the empty rows above are not passing because envelopes + // stopped being read. + renderNode({ hidden: { dialect: 'cel', source: ' true ' } }); + expect(rendered()).toBe(false); + }); +}); + +describe('SchemaRenderer `hidden` chain precedence (objectui#3955)', () => { + beforeEach(() => { + ComponentRegistry.register('probe-3955', Probe as never); + }); + afterEach(() => { + ComponentRegistry.unregister?.('probe-3955'); + }); + + it('an UNDECLARED `hidden` no longer short-circuits — a declared `hiddenOn` is consulted', () => { + // Before: `'' !== undefined` won the chain, `evaluateCondition('')` was + // `true`, and the node vanished for a reason no key stated — `hiddenOn: false` + // was unreachable. This is a behaviour change (alias precedence), pinned as + // such rather than claimed as an equivalence. + renderNode({ hidden: '', hiddenOn: false }); + expect(rendered()).toBe(true); + }); + + it('… and in the other direction: an undeclared `hidden` lets a HOLDING `hiddenOn` hide', () => { + renderNode({ hidden: { dialect: 'cel', source: '' }, hiddenOn: '${data.archived}' }); + expect(rendered()).toBe(false); + }); + + it('a DECLARED `hidden` still wins over `hiddenOn`', () => { + // The case that stays green if someone "fixes" the defect by deleting the + // `hidden` leg: with the leg gone, `hiddenOn: false` would render the node. + renderNode({ hidden: true, hiddenOn: false }); + expect(rendered()).toBe(false); + }); + + it('the `visible*` legs still come first and still keep `!== undefined`', () => { + // objectui#3850's ruling fenced them off: their `true` is negated, so an empty + // predicate already means "shown", and narrowing them would only change alias + // precedence. `visible: false` therefore beats an empty `hidden`… + const { unmount } = renderNode({ visible: false, hidden: '' }); + expect(rendered()).toBe(false); + unmount(); + // …and an EMPTY `visible` still wins the chain outright, so a declared + // `hidden: true` behind it is never consulted. + renderNode({ visible: '', hidden: true }); + expect(rendered()).toBe(true); + }); +}); diff --git a/packages/react/src/hooks/__tests__/actionPredicate.parity.test.tsx b/packages/react/src/hooks/__tests__/actionPredicate.parity.test.tsx index d51677c7c..9a3e64487 100644 --- a/packages/react/src/hooks/__tests__/actionPredicate.parity.test.tsx +++ b/packages/react/src/hooks/__tests__/actionPredicate.parity.test.tsx @@ -36,11 +36,16 @@ * untouched by #3367: sharing a normalizer does not by itself prove the * engine and the renderer reach the same verdict, because they run the * normalized predicate through different call paths. + * 3. The two paths agree on the DECLARED-GATE question as well as the verdict + * (objectui#3957 — the suite at the bottom of this file). A face is two + * questions, "is a gate declared?" then "what does it say?", and the engine + * answered the first one with a range of its own until then: `visible: 0` was + * hidden by the engine and shown by every renderer. */ import { describe, it, expect, vi, afterEach } from 'vitest'; import { renderHook } from '@testing-library/react'; -import { ActionEngine, toPredicateInput as coreToPredicateInput } from '@object-ui/core'; +import { ActionEngine, hasDeclaredPredicate, toPredicateInput as coreToPredicateInput } from '@object-ui/core'; import { toPredicateInput, useCondition } from '../useExpression'; describe('action predicate normalization — one implementation, not two (#3314 / #3367)', () => { @@ -162,3 +167,120 @@ describe('action `visible` — engine path vs renderer path parity (#3314)', () expect(fromEngine).toBe(fromRenderer); }); }); + +/** + * objectui#3957 — the DECLARED-GATE half of the same parity claim. + * + * The suite above compares VERDICTS, which is only half of what a face decides. + * The renderer face is two questions: `hasDeclaredVisibilityGate(schema.visible) + * && !isVisible` — a gate is consulted only when one is declared. The engine face + * used to ask the first question with a range of its own (three empty spellings + * folded by hand, everything else coerced with `Boolean(raw)`), so one value got + * two answers depending on which entry read it: + * + * value | engine (before) | renderer face | agree? + * 0 | HIDDEN | shown | no + * NaN | HIDDEN | shown | no + * ' ' (blank) | HIDDEN | shown | no + * {} / '' | shown | shown | yes + * {cel, source:''} | shown | shown | yes + * + * That is the shape objectui#3314's invariant forbids, and the `' '` row was + * created by objectui#3966 the day it landed: the ruling made a blank predicate + * "no gate" on the renderer face while the engine kept evaluating `'${ }'` to a + * falsy verdict. objectui#3957 moved the engine onto the same definition. + * + * Why `rendererVerdict` alone cannot pin this: it models only the second question. + * For `visible: ' '` it returns `false` (the normalizer wraps a blank string into + * `'${ }'`, whose verdict is falsy) even though the real renderer SHOWS the + * action, because the declared gate in front of it never consults that verdict. + * `rendererFace` below composes both questions, exactly as `action-button` / + * `action-menu` / `DeclaredActionsBar` do. + * + * ## Reverse verification (direction predicted before running) + * + * Restoring the engine's hand-rolled range must turn RED the `0` / `NaN` / + * `' '` / `'\t\n'` / blank-`source`-without-dialect rows of the table below — + * `engine` becomes `false` while `rendererFace` stays `true` — and leave the rows + * both ranges agreed on GREEN. It cannot go red in the other direction: the change + * only ever stops the engine hiding an action. + */ +describe('action `visible` — the DECLARED gate agrees on both faces too (objectui#3957)', () => { + const CONTEXT = { record: { id: 'r1', status: 'open' } }; + + /** The engine face: what `getActionsForLocation` surfaces. */ + function engineFace(visible: unknown): boolean { + const engine = new ActionEngine({ ...CONTEXT }); + engine.registerAction( + { name: 'probe', type: 'api', visible } as never, + { locations: ['record_section'] }, + ); + return engine.getActionsForLocation('record_section').length === 1; + } + + /** + * The renderer face, BOTH questions: `hasDeclaredVisibilityGate(visible) && + * !isVisible` → hidden. This is the composition every action leaf performs. + */ + function rendererFace(visible: unknown): boolean { + if (!hasDeclaredPredicate(visible)) return true; + const { result } = renderHook(() => + useCondition(toPredicateInput(visible), { ...CONTEXT }, { + throwOnError: true, + label: 'action "probe" (visible)', + }), + ); + return result.current; + } + + const GATE_CASES: Array<{ what: string; visible: unknown; shown: boolean }> = [ + // ── nothing declared → shown on both faces ──────────────────────────── + { what: "'' (empty predicate)", visible: '', shown: true }, + { what: 'null', visible: null, shown: true }, + { what: "' ' (blank predicate text — the row objectui#3966 created)", visible: ' ', shown: true }, + { what: "'\\t\\n' (other blanks)", visible: '\t\n', shown: true }, + { what: '0 (not a predicate)', visible: 0, shown: true }, + { what: 'NaN (not a predicate)', visible: NaN, shown: true }, + { what: '{} (no source)', visible: {}, shown: true }, + { what: "{ dialect: 'cel', source: '' } (what `objectstack build` emits)", visible: { dialect: 'cel', source: '' }, shown: true }, + { what: "{ dialect: 'cel', source: ' ' } (blank source — objectui#3960)", visible: { dialect: 'cel', source: ' ' }, shown: true }, + { what: "{ source: ' ' } (blank source, no dialect)", visible: { source: ' ' }, shown: true }, + // ── declared → the verdict decides, identically on both faces ────────── + { what: 'true', visible: true, shown: true }, + { what: 'false (a verdict, not a missing gate)', visible: false, shown: false }, + { what: 'a holding predicate', visible: 'record.status == "open"', shown: true }, + { what: 'a failing predicate', visible: 'record.status == "closed"', shown: false }, + { what: 'a holding CEL envelope', visible: { dialect: 'cel', source: 'record.status == "open"' }, shown: true }, + { what: 'a failing CEL envelope', visible: { dialect: 'cel', source: 'record.status == "closed"' }, shown: false }, + // Blankness is `trim()`, not "short" — the anti-mutation row for the two + // blank-source rows above. + { what: 'a failing CEL envelope padded with blanks', visible: { dialect: 'cel', source: ' record.status == "closed" ' }, shown: false }, + ]; + + it.each(GATE_CASES)('$what → shown=$shown on the engine face AND the renderer face', ({ visible, shown }) => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + const engine = engineFace(visible); + const renderer = rendererFace(visible); + expect(engine).toBe(shown); + expect(renderer).toBe(shown); + // The invariant itself, asserted as such: one value, one answer, whichever + // entry reads it (objectui#3314). + expect(engine).toBe(renderer); + }); + + it('the declared-gate question is asked with ONE definition, not two ranges', () => { + // What makes the table above a convergence rather than a coincidence: both + // faces call `hasDeclaredPredicate`, so there is no second range left to + // drift. Enumerating shapes cannot show that — this can. + for (const c of GATE_CASES) { + const declared = hasDeclaredPredicate(c.visible); + // Every "shown because nothing is declared" row must be undeclared, and + // every row whose verdict decides must be declared. A row that is shown for + // BOTH reasons (e.g. `true`) is declared, so the check is one-directional. + if (!declared) { + expect(c.shown, `${c.what}: nothing declared must mean shown`).toBe(true); + } + } + expect(GATE_CASES.filter(c => !hasDeclaredPredicate(c.visible)).length).toBe(10); + }); +});