From c2ab30bf206b685e3fab052a45e859cca508643b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 20:27:40 +0000 Subject: [PATCH] =?UTF-8?q?fix(app-shell,components):=20=E5=8A=A8=E4=BD=9C?= =?UTF-8?q?=20disabled=20=E7=9A=84=E3=80=8C=E5=B7=B2=E5=A3=B0=E6=98=8E?= =?UTF-8?q?=E3=80=8D=E5=88=A4=E5=AE=9A=E8=A1=A5=E4=B8=8A=20!=3D=3D=20''=20?= =?UTF-8?q?=E5=8D=8A=E8=BE=B9,disabled:=20''=20=E4=B8=8D=E5=86=8D=E6=B0=B8?= =?UTF-8?q?=E4=B9=85=E7=BD=AE=E7=81=B0=20(#3842)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `disabled` 的「是否声明了门」停在 `!= null`,少了 `visible` 一族已收敛的 `hasDeclaredVisibilityGate`(`!= null && !== ''`)的另一半。于是 `disabled: ''` 被判成「声明了门」,verdict 交给求值入口 —— 而求值入口把空谓词读成 「没有条件 → true」(`toPredicateInput('')` → `undefined` → `evaluateCondition(undefined)` → `true`)。 方向是这半边成为缺陷、而 `visible` 那半边不是的原因:在 `visible` 上这个 `true` 意味着「显示」,过宽的「已声明」判定与宽松的空谓词互相抵消,`visible: ''` 两种写法 都渲染;在 `disabled` 上同一个 `true` 意味着「禁用」,两个错误叠加 —— 空谓词从 「没有门」变成「永久置灰」。同一个空谓词,两个 key 得到相反待遇。 两处落点改读同一处定义: - `app-shell` 的 `DeclaredActionsBar`:最烫的一处。动作是服务端声明的 (`objectDef.actions[]`),宿主是审批收件箱记录区,元数据里一个 `disabled: ''` (授权表单留空、模板求值成空串)渲染出谁都点不动的 Approve / Reject,且无法与 「元数据本意」区分。#3835 是同一surface 反方向的失败。 - `components` 的 `action:button`:改前先自行探针实证同形(issue 只按拼法逐字相同 推断,未单独实证):`disabled: ''` 时渲染出的按钮带 `disabled=""`,pin 测试红。 行为变化面刻意窄:只有 `disabled: ''` 改变 —— 从置灰变可点,即「没有谓词」本来的 含义。`disabled: true` 仍置灰,`disabled: false` 与未声明仍不置灰,任何表达式取值的 `disabled` 判定不变。一处连带后果写明:`action:button` 上空的 `disabled` 现在会 **穿透**到 legacy 的 `enabled` 回退腿,而不再在空谓词上短路,所以同时写了 `disabled: '' + enabled: true` 的动作变为可点。 `action:button` 的 legacy `enabled` 腿也改读同一定义,但那部分是按推演 **行为等价**、不是修复:该腿取反(`disabled = !isEnabled`),空谓词的 `true` 本来 就落成「不置灰」,与「没有门」同一结果;四形状在两种判定下逐一相同。推演表与 「为什么没有任何测试能区分这条腿的改动」一并写在 pin 旁边。 `hasDeclaredVisibilityGate` 在两处沿用历史名(#3842 派发裁定):谓词与 key 无关, 同一实现两个名字即轻度方言;两处落点各就地注释说明。 --- .../action-declared-disabled-gate-3842.md | 52 ++++++ .../src/views/DeclaredActionsBar.tsx | 18 +- .../__tests__/DeclaredActionsBar.test.tsx | 83 +++++++++ .../action-disabled-declared-gate.test.tsx | 158 ++++++++++++++++++ .../src/renderers/action/action-button.tsx | 24 ++- 5 files changed, 332 insertions(+), 3 deletions(-) create mode 100644 .changeset/action-declared-disabled-gate-3842.md create mode 100644 packages/components/src/renderers/action/__tests__/action-disabled-declared-gate.test.tsx diff --git a/.changeset/action-declared-disabled-gate-3842.md b/.changeset/action-declared-disabled-gate-3842.md new file mode 100644 index 000000000..57d743d7c --- /dev/null +++ b/.changeset/action-declared-disabled-gate-3842.md @@ -0,0 +1,52 @@ +--- +"@object-ui/app-shell": patch +"@object-ui/components": patch +--- + +An action declaring `disabled: ''` is no longer greyed out forever (objectui#3842) + +The "is a `disabled` gate declared?" test stopped at `!= null`, missing the +`!== ''` half of the invariant the `visible` family converged on +(`hasDeclaredVisibilityGate`, objectui#3492 / #3758 / #3812 / #3823 / #3835). So +`disabled: ''` counted as a declared gate, and the verdict went to the evaluation +entry — which reads an empty predicate as "no condition → `true`" +(`toPredicateInput('')` is `undefined`, `evaluateCondition(undefined)` is `true`). + +The direction is why this half is a defect and the `visible` half was not. On +`visible`, that `true` means SHOW, so an over-broad "declared" test and a +permissive empty predicate cancel out and `visible: ''` renders either way. On +`disabled`, the same `true` means DISABLE — the two mistakes compound, and an +empty predicate stopped meaning "no gate" and started meaning "permanently +greyed out". One empty predicate, opposite treatment under two keys. + +Two gates now ask the shared definition instead: + +- `@object-ui/app-shell`'s `DeclaredActionsBar` — the hot one. Its actions are + SERVER-declared (`objectDef.actions[]`) and its hosts are the approvals inbox's + record sections, so a `disabled: ''` arriving from metadata (an authoring form + left empty, a template that rendered to an empty string) produced an Approve / + Reject button nobody could click, indistinguishable from deliberate metadata. + objectui#3835 was this same surface failing the other way. +- `@object-ui/components`' `action:button` — verified to be the same shape before + it was changed (the issue inferred it from the identical spelling but did not + probe it): with `disabled: ''` the rendered button carried `disabled=""`. + +**Behaviour change surface, deliberately narrow.** Only `disabled: ''` changes — +from disabled to clickable, which is what "no predicate" asked for. `disabled: +true` still disables, `disabled: false` and an absent `disabled` still do not, and +no expression-valued `disabled` changes verdict. One consequence worth naming: on +`action:button`, an empty `disabled` now falls THROUGH to the legacy non-spec +`enabled` fallback instead of short-circuiting on the empty predicate, so an +action spelling both (`disabled: ''` + `enabled: true`) becomes clickable. + +The legacy `enabled` leg of `action:button` was routed through the same +definition for consistency, and that part is behaviour-preserving by derivation +rather than a fix: the leg is negated (`disabled = !isEnabled`), so an empty +predicate's `true` already arrived as "not disabled" — the same verdict "no gate" +produces. All four shapes are identical under either test; the derivation table +and the reason no test can distinguish them are written down next to the pins. + +`hasDeclaredVisibilityGate` keeps its historic name at both call sites (the +objectui#3842 dispatch ruling): the predicate is key-neutral, and one +implementation behind two names is how a repo grows dialects. Each call site says +so in a comment. diff --git a/packages/app-shell/src/views/DeclaredActionsBar.tsx b/packages/app-shell/src/views/DeclaredActionsBar.tsx index 98e73cee8..2390958fd 100644 --- a/packages/app-shell/src/views/DeclaredActionsBar.tsx +++ b/packages/app-shell/src/views/DeclaredActionsBar.tsx @@ -227,7 +227,23 @@ const DeclaredActionButton: React.FC<{ type="button" size="sm" variant={variant as any} - disabled={((action as any).disabled != null ? isDisabledPred : false) || loading} + // Is a `disabled` gate DECLARED? The same question the `visible` gate + // above asks, so it reads the same definition rather than re-spelling it. + // The name is historic — objectui#3492 arrived through `visible` — and the + // predicate is key-neutral: "declared" is `!= null && !== ''`, because an + // empty predicate is nothing to evaluate. Kept under that name + // deliberately (objectui#3842 ruling): one implementation behind two names + // is a dialect, not a clarification. + // + // `!= null` alone was a real defect here, and NOT for the reason it was on + // `visible`: the evaluation entry reads an empty predicate as "no + // condition → true", which on `visible` means SHOW (so an over-broad + // "declared" test cancels out and `''` renders either way), but here means + // DISABLE. A `disabled: ''` on a server-declared approval action rendered + // a permanently greyed-out Approve / Reject — the mirror image of + // objectui#3835 on the same surface, and equally impossible to tell from + // deliberate metadata by looking at it. + disabled={(hasDeclaredVisibilityGate((action as any).disabled) ? isDisabledPred : false) || loading} onClick={handleClick} data-testid={`declared-action-${action.name}`} > diff --git a/packages/app-shell/src/views/__tests__/DeclaredActionsBar.test.tsx b/packages/app-shell/src/views/__tests__/DeclaredActionsBar.test.tsx index 59187399b..60a93a1a4 100644 --- a/packages/app-shell/src/views/__tests__/DeclaredActionsBar.test.tsx +++ b/packages/app-shell/src/views/__tests__/DeclaredActionsBar.test.tsx @@ -529,3 +529,86 @@ describe('DeclaredActionsBar — declared `visible` on a server-declared action expect(executeSpy.mock.calls[0][0]).toMatchObject({ name: 'approval_reassign' }); }); }); + +/** + * objectui#3842 — the declared `disabled` gate on the same surface, and the + * half of the objectui#3492 family where the `''` reasoning INVERTS. + * + * The gate asked `(action as any).disabled != null`, so `disabled: ''` counted + * as "a gate is declared" and the verdict went to the evaluation entry — which + * reads an empty predicate as "no condition → true" + * (`toPredicateInput('')` → `undefined` → `evaluateCondition(undefined)` → + * `true`). Direction is everything: on `visible` that `true` means SHOW, so the + * over-broad "declared" test and the permissive empty predicate cancel out + * (which is why the `visible: ''` case above is documentation, not a detector, + * and says so). On `disabled` the same `true` means DISABLE, so the two + * compound: an empty predicate stopped meaning "no gate" and started meaning + * "greyed out forever". + * + * What that costs on THIS surface: the actions are server-declared + * (`objectDef.actions[]`) and the bar's own hosts are the approvals inbox's + * record sections, so a `disabled: ''` — an authoring form left empty, a + * template that rendered to an empty string — greys out Approve / Reject with + * no way for the approver to tell metadata intent from a bug. objectui#3835 was + * the same surface failing the other way (hidden action still clickable). + * + * `''` IS a mutation detector here, unlike on `visible`: restore `!= null` and + * this block's first case goes red on its own. All four shapes are asserted + * because "never disable anything" satisfies three of them, and `disabled: true` + * is what refuses that rewrite. + * + * No legacy `enabled` leg on this surface by design (see the component: server- + * declared actions are spec-shaped and never carried the non-spec key) — that + * leg's four shapes are pinned in `packages/components`, next to the renderer + * that has it. + */ +describe('DeclaredActionsBar — declared `disabled` on a server-declared action (objectui#3842)', () => { + const APPROVE = { + name: 'approval_approve', + type: 'api', + label: 'Approve', + target: '/api/v1/approvals/requests/{id}/approve', + locations: ['record_section'], + }; + const approve = () => screen.getByTestId('declared-action-approval_approve'); + + it("an empty-string `disabled` is not a declared gate — Approve stays clickable", () => { + renderWithGate({ ...APPROVE, disabled: '' }); + expect(approve()).not.toBeDisabled(); + }); + + it('disabled:true → Approve is disabled', () => { + renderWithGate({ ...APPROVE, disabled: true }); + expect(approve()).toBeDisabled(); + }); + + it('disabled:false → Approve is not disabled', () => { + renderWithGate({ ...APPROVE, disabled: false }); + expect(approve()).not.toBeDisabled(); + }); + + it('no `disabled` at all → Approve is not disabled (ungated stays ungated)', () => { + renderWithGate({ ...APPROVE }); + expect(approve()).not.toBeDisabled(); + }); + + it('an expression-valued `disabled` keeps its verdict — true disables, false does not', () => { + const gated = { ...APPROVE, disabled: 'status == "approved"' }; + const { unmount } = renderWithGate(gated, { ...REQUEST, status: 'approved' }); + expect(approve()).toBeDisabled(); + unmount(); + renderWithGate(gated, { ...REQUEST, status: 'pending' }); + expect(approve()).not.toBeDisabled(); + }); + + it('an empty-string `disabled` leaves an Approve that actually DISPATCHES', async () => { + // Not-disabled is only the visible half of the claim. The `disabled` + // attribute is what this component hands its own click handler — the one + // that POSTs the approve call — so the case worth pinning is that the + // button reaches `execute`, not merely that an attribute is absent. + renderWithGate({ ...APPROVE, disabled: '' }); + fireEvent.click(approve()); + await waitFor(() => expect(executeSpy).toHaveBeenCalledTimes(1)); + expect(executeSpy.mock.calls[0][0]).toMatchObject({ name: 'approval_approve' }); + }); +}); diff --git a/packages/components/src/renderers/action/__tests__/action-disabled-declared-gate.test.tsx b/packages/components/src/renderers/action/__tests__/action-disabled-declared-gate.test.tsx new file mode 100644 index 000000000..bedc4ba27 --- /dev/null +++ b/packages/components/src/renderers/action/__tests__/action-disabled-declared-gate.test.tsx @@ -0,0 +1,158 @@ +/** + * 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#3842 — the declared-`disabled` gate. The `disabled` half of the + * objectui#3492 family, and the half where the family's usual "`''` is + * harmless" reasoning INVERTS. + * + * The gate asked `(schema as any).disabled != null`, so `disabled: ''` counted + * as "a gate is declared" and the verdict was handed to the evaluation entry — + * which reads an empty predicate as "nothing to evaluate → true" + * (`toPredicateInput('')` is `undefined`, `evaluateCondition(undefined)` is + * `true`). On `visible` that `true` means SHOW, so an over-broad "declared" + * test and a permissive empty predicate cancel out and `visible: ''` renders + * either way (which is why PR #3843 could only document `''` on that side, not + * detect a mutation with it). On `disabled` the same `true` means DISABLE, so + * the two mistakes compound instead of cancelling: an empty predicate stopped + * meaning "no gate" and started meaning "greyed out forever". + * + * Both legs now ask `hasDeclaredVisibilityGate` (`!= null && !== ''`) — the one + * definition, imported rather than re-spelled, historic name kept per the + * objectui#3842 dispatch ruling (see the comment at the gate). + * + * ## What each case detects + * + * • `disabled: ''` → NOT disabled. THE defect, and on this side a genuine + * mutation detector: restore `!= null` and this case alone goes red. + * • `disabled: true` → disabled, and `disabled: false` / undeclared → not + * disabled. Anti-mutation guards: "never disable anything" satisfies three + * of the four shapes on its own, and `true` is what refuses it. + * • expression-valued `disabled` → the verdict still decides, both ways. The + * gate narrowed; evaluation did not change. + * + * ## The legacy `enabled` leg, and why its four cases are documentation + * + * The leg is NEGATED (`disabled = !isEnabled`), so the empty predicate's `true` + * arrives as `!true` = "not disabled" — which is exactly what "no gate + * declared" produces. Every shape therefore reaches the same verdict under + * either test, and the tightening is behaviour-preserving by derivation: + * + * | `enabled` | `!= null` (old) | `hasDeclaredVisibilityGate` (new) | + * |-------------|--------------------------|-----------------------------------| + * | `''` | gate → `!true` = enabled | no gate → `false` = enabled | + * | `true` | gate → `!true` = enabled | gate → `!true` = enabled | + * | `false` | gate → `!false` = DISABLED| gate → `!false` = DISABLED | + * | undeclared | no gate → `false` | no gate → `false` | + * + * So no `enabled` case here can go red by reverting the `enabled` leg — stated + * plainly rather than dressed up as coverage. They are kept because they pin + * the semantics the derivation asserts (`enabled: false` must still disable), + * which is what a future rewrite of this chain would break silently. The one + * case that DOES move is the precedence case below: with `disabled: ''` no + * longer a gate, the chain falls through to the legacy leg instead of + * short-circuiting on an empty predicate. + */ + +import { describe, it, expect } 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 { 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 §测试纪律 — the cost lands in +// the import phase, not under a hook timeout. +import '../action-button'; + +/** Mount the leaf the way `action:bar` mounts it: whole action spread onto `schema`. */ +function renderLeaf(action: any, 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' }); + +describe('action:button — declared `disabled` gate (objectui#3842)', () => { + it("an empty-string `disabled` is not a declared gate — the button stays clickable", () => { + renderLeaf({ ...ACT, disabled: '' }); + expect(act()).not.toBeDisabled(); + }); + + it('disabled:true → the button is disabled', () => { + renderLeaf({ ...ACT, disabled: true }); + expect(act()).toBeDisabled(); + }); + + it('disabled:false → the button is not disabled', () => { + renderLeaf({ ...ACT, disabled: false }); + expect(act()).not.toBeDisabled(); + }); + + it('no `disabled` at all → the button is not disabled (ungated stays ungated)', () => { + renderLeaf({ ...ACT }); + expect(act()).not.toBeDisabled(); + }); + + it('an expression-valued `disabled` keeps its verdict — true disables, false does not', () => { + 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 — legacy `enabled` leg (objectui#3842)', () => { + it("an empty-string `enabled` is not a declared gate — the button stays clickable", () => { + renderLeaf({ ...ACT, enabled: '' }); + expect(act()).not.toBeDisabled(); + }); + + it('enabled:false → the button is disabled (the legacy leg still decides)', () => { + renderLeaf({ ...ACT, enabled: false }); + expect(act()).toBeDisabled(); + }); + + it('enabled:true → the button is not disabled', () => { + renderLeaf({ ...ACT, enabled: true }); + expect(act()).not.toBeDisabled(); + }); + + it('no `enabled` at all → the button is not disabled', () => { + renderLeaf({ ...ACT }); + expect(act()).not.toBeDisabled(); + }); + + it('an empty `disabled` falls THROUGH to the legacy `enabled` leg', () => { + // The precedence case, and the second one the narrowing moves: `disabled: + // ''` used to short-circuit the chain on an empty predicate (→ disabled), + // so the author's `enabled: true` was never consulted. With `''` no longer + // a gate, the legacy leg decides — "no `disabled` declared" means exactly + // that, whatever else the action declares. + renderLeaf({ ...ACT, disabled: '', enabled: true }); + expect(act()).not.toBeDisabled(); + }); + + it('an empty `disabled` does not mask a legacy `enabled: false`', () => { + // Same fall-through, opposite verdict: the legacy leg is reached and says + // disabled. Green before and after (the old chain also disabled, for the + // wrong reason) — it is here so the fall-through cannot be "read the + // `enabled` leg only when it agrees with not-disabled". + renderLeaf({ ...ACT, disabled: '', enabled: false }); + expect(act()).toBeDisabled(); + }); +}); diff --git a/packages/components/src/renderers/action/action-button.tsx b/packages/components/src/renderers/action/action-button.tsx index 274b555da..8a967dfc9 100644 --- a/packages/components/src/renderers/action/action-button.tsx +++ b/packages/components/src/renderers/action/action-button.tsx @@ -160,10 +160,30 @@ const ActionButtonRenderer = forwardRef( variant={variant as any} size={size as any} className={cn(schema.className, className)} + // Is a `disabled` / `enabled` gate DECLARED? Same question as the + // `visible` gate above, so it reads the same definition — the name is + // historic (objectui#3492 arrived through `visible`); the predicate is + // key-neutral: "declared" means `!= null && !== ''`, an empty predicate + // is nothing to evaluate and therefore no gate. Deliberately NOT + // renamed or aliased for this call site (objectui#3842 ruling): one + // implementation under two names is how a repo grows dialects. + // + // `!= null` alone was a live defect here in a way it is not on + // `visible` (objectui#3842). The evaluation entry reads an empty + // predicate as "no condition → true"; on `visible` that `true` means + // SHOW, so an over-broad "declared" test cancels out, while here it + // means DISABLE — `disabled: ''` was a permanently greyed-out button. + // + // The legacy `enabled` leg is negated (`!isEnabled`), so its `''` case + // already landed on "not disabled" by double negation; routing it + // through the same definition is behaviour-preserving for all four + // shapes (derivation table in + // `__tests__/action-disabled-declared-gate.test.tsx`) and keeps one + // spelling of "declared" on both legs. disabled={( - (schema as any).disabled != null + hasDeclaredVisibilityGate((schema as any).disabled) ? isDisabled - : schema.enabled != null + : hasDeclaredVisibilityGate(schema.enabled) ? !isEnabled : false ) || loading}