Skip to content

Commit 7b21891

Browse files
os-zhuangclaude
andauthored
fix(action): honor the spec disabled predicate on every action-rendering surface (#1885 follow-through) (#2863)
Spec Action field is `disabled` (boolean | CEL, disabled when TRUE); the schema has no `enabled` key. #1885 wired action:button only. Browser dogfooding found five more surfaces where an authored `disabled` silently did nothing: - components: action:group leaves (inline + dropdown), action:icon, action:menu read the legacy non-spec `enabled` only. `disabled` is now primary (same evaluation scope as `visible`), `enabled` stays as a deprecated fallback. - app-shell: DeclaredActionsBar read neither; gains `disabled` (no legacy fallback -- declared actions are spec-shaped). - plugin-detail: record:quick_actions HAD a disabled implementation but its `typeof === 'string'` split dropped the {dialect:'cel', source} envelope the server compiles authored CEL into (#2661), so it never fired on real metadata. Now feeds toPredicateInput's result to useCondition whole. Pinned by new DropdownActionItem tests (4 cases); browser-verified against the showcase showcase_archive_task specimen: greyed on an in-progress task, clickable on a done one. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 09c6a17 commit 7b21891

7 files changed

Lines changed: 127 additions & 11 deletions

File tree

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
"@object-ui/components": patch
3+
"@object-ui/app-shell": patch
4+
"@object-ui/plugin-detail": patch
5+
---
6+
7+
fix(action): honor the spec `disabled` predicate on every action-rendering surface (#1885 follow-through)
8+
9+
The spec Action field is `disabled` (boolean | CEL — disabled when TRUE); the
10+
schema has no `enabled` key. #1885 wired it in `action:button` only. Browser
11+
dogfooding against the showcase found FIVE more surfaces where a spec-authored
12+
`disabled` silently did nothing:
13+
14+
- **components** — the `action:group` leaves (inline + dropdown), `action:icon`
15+
and `action:menu` still read the legacy non-spec `enabled`. They now consume
16+
`disabled` as the primary control (evaluated in the same scope as `visible`),
17+
with `enabled` kept as a deprecated fallback.
18+
- **app-shell**`DeclaredActionsBar` (server-declared action bar) read
19+
neither; it gains `disabled` (no legacy fallback: declared actions are
20+
spec-shaped and never carried `enabled`).
21+
- **plugin-detail**`record:quick_actions` HAD a `disabled` implementation,
22+
but its `typeof === 'string'` split dropped the `{dialect:'cel', source}`
23+
envelope the server compiles authored CEL into (#2661 routes envelopes to the
24+
canonical formula engine), so the predicate never fired on real metadata. It
25+
now feeds `toPredicateInput`'s result to `useCondition` whole, like every
26+
other surface.
27+
28+
Pinned by new `DropdownActionItem` tests (disabled-when-TRUE, false-stays-
29+
clickable, disabled-wins-over-enabled, boolean literal) and browser-verified
30+
end-to-end against the showcase `showcase_archive_task` specimen: greyed on an
31+
in-progress task, clickable on a done one (with `visible` hiding Mark Done on
32+
the same screen — the hide-vs-grey contrast).

packages/app-shell/src/views/DeclaredActionsBar.tsx

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,12 @@ const DeclaredActionButton: React.FC<{
112112
throwOnError: true,
113113
label: `declared action "${action.name ?? action.label ?? 'action'}" (visible)`,
114114
});
115+
// Spec `disabled` (boolean | CEL — disabled when TRUE), evaluated against the
116+
// same record context as `visible`. #1885 wired it in action-button only;
117+
// this bar ignored it, so a spec-authored `disabled` guard on a declared
118+
// action did nothing here. (No legacy `enabled` fallback: server-declared
119+
// actions are spec-shaped and never carried the non-spec key.)
120+
const isDisabledPred = useCondition(toPredicateInput((action as any).disabled), recordData);
115121

116122
const handleClick = useCallback(async () => {
117123
if (loading) return;
@@ -219,7 +225,7 @@ const DeclaredActionButton: React.FC<{
219225
type="button"
220226
size="sm"
221227
variant={variant as any}
222-
disabled={loading}
228+
disabled={((action as any).disabled != null ? isDisabledPred : false) || loading}
223229
onClick={handleClick}
224230
data-testid={`declared-action-${action.name}`}
225231
>

packages/components/src/renderers/action/__tests__/action-group-dropdown-visible.test.tsx

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,3 +70,43 @@ describe('action:group dropdown item — visible / enabled CEL', () => {
7070
expect(item).toHaveAttribute('data-disabled');
7171
});
7272
});
73+
74+
// Spec field is `disabled` (boolean | CEL — disabled when TRUE). #1885 wired it
75+
// in action-button only; the group/icon/menu leaves kept reading the legacy
76+
// non-spec `enabled`, so a spec-authored `disabled` guard silently did nothing
77+
// there. These pin the follow-through: `disabled` is the primary control and
78+
// wins over the deprecated `enabled` fallback.
79+
describe('action:group dropdown item — spec `disabled` CEL', () => {
80+
it('disables an action whose `disabled` predicate is TRUE', () => {
81+
renderItem(
82+
{ name: 'close', label: 'Close', disabled: 'record.status == "closed"' },
83+
{ record: { status: 'closed' } },
84+
);
85+
const item = screen.getByText('Close').closest('[role="menuitem"]');
86+
expect(item).toHaveAttribute('data-disabled');
87+
});
88+
89+
it('keeps an action clickable when `disabled` is FALSE', () => {
90+
renderItem(
91+
{ name: 'close', label: 'Close', disabled: 'record.status == "closed"' },
92+
{ record: { status: 'open' } },
93+
);
94+
const item = screen.getByText('Close').closest('[role="menuitem"]');
95+
expect(item).not.toHaveAttribute('data-disabled');
96+
});
97+
98+
it('`disabled` wins over the legacy `enabled` fallback when both are present', () => {
99+
renderItem(
100+
{ name: 'close', label: 'Close', disabled: 'record.locked == true', enabled: 'true' },
101+
{ record: { locked: true } },
102+
);
103+
const item = screen.getByText('Close').closest('[role="menuitem"]');
104+
expect(item).toHaveAttribute('data-disabled');
105+
});
106+
107+
it('supports the boolean literal form (`disabled: true`)', () => {
108+
renderItem({ name: 'close', label: 'Close', disabled: true });
109+
const item = screen.getByText('Close').closest('[role="menuitem"]');
110+
expect(item).toHaveAttribute('data-disabled');
111+
});
112+
});

packages/components/src/renderers/action/action-group.tsx

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,11 @@ const InlineActionButton: React.FC<{
6969
}> = ({ action, variant, size, onExecute }) => {
7070
const [loading, setLoading] = useState(false);
7171
const isVisible = useCondition(toPredicateInput(action.visible));
72+
// Spec field is `disabled` (boolean | CEL — disabled when TRUE). #1885 wired
73+
// it in action-button only; this leaf kept reading the legacy non-spec
74+
// `enabled`, so a spec-authored `disabled` guard did nothing here. `disabled`
75+
// is now the primary control; `enabled` stays as a deprecated fallback.
76+
const isDisabledPred = useCondition(toPredicateInput((action as any).disabled));
7277
const isEnabled = useCondition(toPredicateInput(action.enabled));
7378

7479
const Icon = resolveIcon(action.icon);
@@ -93,7 +98,13 @@ const InlineActionButton: React.FC<{
9398
variant={btnVariant as any}
9499
size={btnSize as any}
95100
className={action.className}
96-
disabled={(action.enabled ? !isEnabled : false) || loading}
101+
disabled={(
102+
(action as any).disabled != null
103+
? isDisabledPred
104+
: action.enabled != null
105+
? !isEnabled
106+
: false
107+
) || loading}
97108
onClick={handleClick}
98109
>
99110
{loading && <Loader2 className="mr-2 h-4 w-4 animate-spin" />}
@@ -122,10 +133,17 @@ export const DropdownActionItem: React.FC<{
122133
onSelect: (action: ActionSchema) => void | Promise<void>;
123134
}> = ({ action, index, onSelect }) => {
124135
const isVisible = useCondition(toPredicateInput(action.visible));
136+
// Spec `disabled` primary, legacy non-spec `enabled` fallback (see
137+
// InlineActionButton above — #1885 follow-through).
138+
const isDisabledPred = useCondition(toPredicateInput((action as any).disabled));
125139
const isEnabled = useCondition(toPredicateInput(action.enabled));
126140
if (action.visible && !isVisible) return null;
127141
const Icon = resolveIcon(action.icon);
128-
const isDisabled = action.enabled ? !isEnabled : false;
142+
const isDisabled = (action as any).disabled != null
143+
? isDisabledPred
144+
: action.enabled != null
145+
? !isEnabled
146+
: false;
129147
const showSeparator = action.tags?.includes('separator-before') && index > 0;
130148
return (
131149
<>

packages/components/src/renderers/action/action-icon.tsx

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ const ActionIconRenderer = forwardRef<HTMLButtonElement, ActionIconProps>(
4343
const [loading, setLoading] = useState(false);
4444

4545
const isVisible = useCondition(toPredicateInput(schema.visible));
46+
// Spec `disabled` (boolean | CEL — disabled when TRUE) primary, legacy
47+
// non-spec `enabled` fallback (#1885 follow-through — only action-button
48+
// was wired; this renderer ignored a spec-authored `disabled`).
49+
const isDisabledPred = useCondition(toPredicateInput((schema as any).disabled));
4650
const isEnabled = useCondition(toPredicateInput(schema.enabled));
4751

4852
const Icon = resolveIcon(schema.icon);
@@ -85,7 +89,13 @@ const ActionIconRenderer = forwardRef<HTMLButtonElement, ActionIconProps>(
8589
variant={variant as any}
8690
size={size}
8791
className={cn('h-8 w-8', schema.className, className)}
88-
disabled={(schema.enabled ? !isEnabled : false) || loading}
92+
disabled={(
93+
(schema as any).disabled != null
94+
? isDisabledPred
95+
: schema.enabled != null
96+
? !isEnabled
97+
: false
98+
) || loading}
8999
onClick={handleClick}
90100
aria-label={schema.label || schema.name}
91101
{...rest}

packages/components/src/renderers/action/action-menu.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,10 @@ const ActionMenuItem: React.FC<{
6969
throwOnError: true,
7070
label: `action "${action.name ?? action.label ?? 'action:menu item'}" (visible)`,
7171
});
72+
// Spec `disabled` (boolean | CEL — disabled when TRUE) primary, legacy
73+
// non-spec `enabled` fallback (#1885 follow-through — only action-button
74+
// was wired; this renderer ignored a spec-authored `disabled`).
75+
const isDisabledPred = useCondition(toPredicateInput((action as any).disabled));
7276
const isEnabled = useCondition(toPredicateInput(action.enabled));
7377

7478
const iconElement = useMemo(() => {
@@ -81,7 +85,11 @@ const ActionMenuItem: React.FC<{
8185

8286
return (
8387
<DropdownMenuItem
84-
disabled={action.enabled ? !isEnabled : false}
88+
disabled={(action as any).disabled != null
89+
? isDisabledPred
90+
: action.enabled != null
91+
? !isEnabled
92+
: false}
8593
onSelect={(e) => {
8694
e.preventDefault();
8795
onExecute(action);

packages/plugin-detail/src/renderers/record-quick-actions.tsx

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -198,12 +198,14 @@ function QuickActionButton({
198198
}) {
199199
const [running, setRunning] = React.useState(false);
200200
const recordCtx = React.useMemo(() => ({ record: record || {} }), [record]);
201-
// `disabled` may be a boolean or a CEL predicate. Evaluate the predicate form
202-
// against the record; useCondition must be called unconditionally (hook).
203-
const disabledPred = toPredicateInput((action as any).disabled);
204-
const condDisabled = useCondition(typeof disabledPred === 'string' ? disabledPred : undefined, recordCtx);
205-
const isDisabled =
206-
(typeof disabledPred === 'string' ? condDisabled : disabledPred === true) || running;
201+
// `disabled` may be a boolean or a CEL predicate (disabled when TRUE). Feed
202+
// toPredicateInput's result to useCondition WHOLE — since #2661 a CEL-dialect
203+
// `{dialect, source}` envelope (the shape the server compiles authored CEL
204+
// into) must reach useCondition intact to route to the canonical formula
205+
// engine. The previous `typeof === 'string'` split dropped the envelope, so
206+
// a spec-authored `disabled` never disabled anything on this surface.
207+
const isDisabledPred = useCondition(toPredicateInput((action as any).disabled), recordCtx);
208+
const isDisabled = ((action as any).disabled != null ? isDisabledPred : false) || running;
207209
return (
208210
<Button
209211
variant={variant}

0 commit comments

Comments
 (0)