Skip to content

Commit 5976ba3

Browse files
os-zhuangclaude
andauthored
fix(core): evaluate bare CEL predicates in evaluateCondition (#1788)
`evaluateCondition` delegated to `evaluate`, which only processes `${...}` templates and returns any other string verbatim — so a bare predicate like `record.status == "converted"` (what objectstack build emits for disabled/visible/condition) coerced to `true`, making every bare-expression predicate silently always-truthy. Symptom: a param-collecting api action on the record header (CRM "Reassign Lead") was treated as permanently disabled, so ActionRunner.execute bailed before opening the param dialog. The page:header renderer was unaffected because it uses evaluateExpression directly. - evaluateCondition: treat a non-`${}` condition as a single expression via evaluateExpression; keep the template path; preserve empty/undefined and unparseable -> visible/enabled fallbacks. - ActionRunner: evaluate the `disabled` gate (boolean/string/envelope) instead of treating any object as truthy. - plugin-grid RowActionMenu: unify the row-action predicate scope so `record.*` and bare-field predicates resolve identically on every surface. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent a00e16d commit 5976ba3

4 files changed

Lines changed: 75 additions & 13 deletions

File tree

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
---
2+
"@object-ui/core": patch
3+
---
4+
5+
fix(core): evaluate bare CEL predicates in `evaluateCondition`
6+
7+
`ExpressionEvaluator.evaluateCondition` delegated to `evaluate`, which only
8+
processes `${...}` templates and returns any other string verbatim. A bare
9+
predicate such as `record.status == "converted"` (the shape `objectstack build`
10+
emits for `disabled`/`visible`/`condition`) was therefore returned as a
11+
non-empty string and coerced to `true` — so every bare-expression predicate was
12+
silently always-truthy.
13+
14+
The most visible symptom: a param-collecting `api` action invoked from the
15+
record header (e.g. CRM "Reassign Lead") was treated as permanently `disabled`,
16+
so `ActionRunner.execute` bailed before opening the param dialog. The renderer
17+
(`page:header`) was unaffected because it evaluates via `evaluateExpression`
18+
directly.
19+
20+
`evaluateCondition` now treats a non-`${}` condition as a single expression
21+
(via `evaluateExpression`), keeps the `${...}` template path, and preserves the
22+
"empty/undefined ⇒ visible/enabled" and "unparseable ⇒ default visible/enabled"
23+
fallbacks. Also hardens `ActionRunner`'s `disabled` gate to evaluate the
24+
boolean/string/envelope form rather than treating any object as truthy, and
25+
unifies the grid row-action predicate scope so `record.*` and bare-field
26+
predicates resolve identically on every surface.

packages/core/src/actions/ActionRunner.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -385,11 +385,21 @@ export class ActionRunner {
385385
}
386386
}
387387

388-
if (action.disabled) {
389-
const isDisabled = typeof action.disabled === 'string'
390-
? this.evaluator.evaluateCondition(action.disabled)
391-
: action.disabled;
392-
388+
if (action.disabled != null && action.disabled !== false) {
389+
// `disabled` may be a boolean, a CEL string, or the normalized envelope
390+
// `{ dialect, source }` (what `objectstack build` emits). The previous
391+
// code only evaluated the STRING form and treated any object as truthy,
392+
// so an envelope-disabled action was ALWAYS "disabled" — silently
393+
// blocking every execution (param dialog never opened, handler never
394+
// ran). `evaluateCondition` already handles boolean/string/envelope;
395+
// and the renderers are authoritative for the visual disabled state, so
396+
// any eval failure here defaults to NOT-disabled (don't false-block).
397+
let isDisabled = false;
398+
try {
399+
isDisabled = this.evaluator.evaluateCondition(action.disabled as never);
400+
} catch {
401+
isDisabled = false;
402+
}
393403
if (isDisabled) {
394404
return { success: false, error: 'Action is disabled' };
395405
}

packages/core/src/evaluator/ExpressionEvaluator.ts

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -213,14 +213,36 @@ export class ExpressionEvaluator {
213213
condition = (condition as any).source as string;
214214
}
215215

216+
// No condition → default to visible/enabled (undefined, null, '').
216217
if (!condition) {
217-
return true; // Default to visible/enabled if no condition
218+
return true;
218219
}
219220

220-
const result = this.evaluate(condition, options);
221-
222-
// Convert result to boolean
223-
return Boolean(result);
221+
if (typeof condition !== 'string') {
222+
return Boolean(condition);
223+
}
224+
225+
const trimmed = condition.trim();
226+
if (!trimmed) {
227+
return true; // Whitespace-only → treat as "no condition".
228+
}
229+
230+
// A condition is semantically a single boolean expression. When it's a
231+
// `${...}` template, evaluate via the template path. Otherwise treat the
232+
// ENTIRE string as one expression (bare CEL like `record.status == "x"`):
233+
// `evaluate` would short-circuit a non-`${}` string and return it verbatim,
234+
// so `Boolean('record.status == "x"')` was ALWAYS true — silently making
235+
// every bare-expression `disabled`/`condition`/`visible` predicate truthy.
236+
if (trimmed.includes('${')) {
237+
return Boolean(this.evaluate(trimmed, options));
238+
}
239+
try {
240+
return Boolean(this.evaluateExpression(trimmed, { sanitize: options.sanitize !== false }));
241+
} catch {
242+
// Unparseable predicate — preserve the historical "default to
243+
// visible/enabled" behaviour rather than hiding/blocking on a typo.
244+
return true;
245+
}
224246
}
225247

226248
/**

packages/plugin-grid/src/components/RowActionMenu.tsx

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,15 @@ const RowActionMenuItem: React.FC<{
9090
row: any;
9191
onActionDef?: (def: RowActionDef, row: any) => void;
9292
}> = ({ def, row, onActionDef }) => {
93-
const isVisible = useCondition(toPredicateInput(def.visible), row);
93+
// Evaluate predicates against the row with BOTH a bare-field scope (`status`)
94+
// and a `record.` scope (`record.status`) so authors can use either
95+
// convention consistently with the record-header / spec evaluators.
96+
const predicateCtx = { ...(row && typeof row === 'object' ? row : {}), record: row };
97+
const isVisible = useCondition(toPredicateInput(def.visible), predicateCtx);
9498
// `disabled` may be a boolean or a CEL predicate evaluated against the row
9599
// (e.g. grey out "Reassign" once a lead is converted) — previously ignored.
96100
const disabledPred = toPredicateInput((def as any).disabled);
97-
const evalDisabled = useCondition(typeof disabledPred === 'string' ? disabledPred : undefined, row);
101+
const evalDisabled = useCondition(typeof disabledPred === 'string' ? disabledPred : undefined, predicateCtx);
98102
const isDisabled = typeof disabledPred === 'string' ? evalDisabled : disabledPred === true;
99103
if (def.visible && !isVisible) return null;
100104
return (
@@ -132,7 +136,7 @@ const RowActionInlineButton: React.FC<{
132136
row: any;
133137
onActionDef?: (def: RowActionDef, row: any) => void;
134138
}> = ({ def, row, onActionDef }) => {
135-
const isVisible = useCondition(toPredicateInput(def.visible), row);
139+
const isVisible = useCondition(toPredicateInput(def.visible), { ...(row && typeof row === 'object' ? row : {}), record: row });
136140
if (def.visible && !isVisible) return null;
137141
return (
138142
<Button

0 commit comments

Comments
 (0)