Skip to content

Commit 9e7349e

Browse files
os-zhuangclaude
andauthored
refactor(actions)!: target is the only handler slot — delete the execute alias (objectstack#3856) (#2990)
`ActionRunner.executeScript` read `action.target || action.execute`. Against `@objectstack/spec` 17 that fallback is unreachable: `execute` is a tombstoned key (objectstack#3855) the parser rejects with the rename prescription, so no parsed action can carry it and the `||` could only ever yield `target`. Deleted rather than kept as harmless residue: two handler slots is what let one action run one script server-side and a different one client-side (objectstack#3713, where this renderer preferred the alias while the spec transform preferred `target`). objectstack#3856 predicted a compile error here and there wasn't one — the real finding. Neither reader was typed against the spec's `z.infer`; both hand-declared the key: - `@object-ui/types` `ActionSchema.execute` removed, so `execute: '…'` now fails `tsc` at the authoring site (TS2353). - `@object-ui/core` `ActionDef.execute` removed too, but `ActionDef` carries a `[key: string]: any` index signature, so stale hand-authored metadata still compiles. For that path `executeScript` returns the rename prescription rather than a bare "No script provided" — removing an authorable key has to be audible, and binding no handler silently is the objectstack#2169 "Mark Done does nothing" shape. The four action renderers stop forwarding `execute`, and Studio's `ActionPreview` no longer falls back to it. Tests move to `target`; the "prefer canonical over the alias" case becomes a regression test that the alias is not read. Closes objectstack-ai/objectstack#3856 Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 3c1f321 commit 9e7349e

14 files changed

Lines changed: 118 additions & 50 deletions

File tree

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
---
2+
'@object-ui/types': major
3+
'@object-ui/core': major
4+
'@object-ui/components': major
5+
'@object-ui/app-shell': major
6+
---
7+
8+
**`target` is the only action handler slot — the `execute` alias is gone from the renderer (framework#3856).**
9+
10+
`ActionRunner.executeScript` read `action.target || action.execute`. That fallback
11+
is unreachable against `@objectstack/spec` 17: `execute` is now a tombstoned key
12+
(framework#3855) that the parser **rejects** with the rename prescription, so no
13+
parsed action can carry it and the `||` could only ever yield `target`. Verified
14+
against 17.0.0-rc.0 — an action declaring `execute` fails `ActionSchema.safeParse`,
15+
and a `target` action's parsed output has no `execute` key at all.
16+
17+
Deleted rather than left as harmless residue: two handler slots is what let one
18+
action run one script server-side and a different one client-side (framework#3713,
19+
where this renderer preferred the alias while the spec transform preferred
20+
`target`). A dead slot still reads as a live contract to the next maintainer.
21+
22+
`execute` is also **removed from the types**, which is the part that had never
23+
landed. framework#3856 predicted a compile error here; there wasn't one, because
24+
neither reader was typed against the spec's `z.infer`:
25+
26+
- `@object-ui/types` `ActionSchema` hand-declared `execute?: string`. Removed, so
27+
`execute: '…'` now fails `tsc` at the authoring site (TS2353).
28+
- `@object-ui/core` `ActionDef` hand-declared it too. Removed — but `ActionDef`
29+
carries a `[key: string]: any` index signature, so stale hand-authored metadata
30+
that never passed through the parser still compiles. For that path
31+
`executeScript` now returns the rename prescription instead of a bare
32+
"No script provided", matching the spec tombstone's rule that removing an
33+
authorable key must be audible: silently binding no handler is the
34+
"Mark Done does nothing" shape (framework#2169).
35+
36+
The four action renderers (`action:button`, `action:icon`, `action:menu`,
37+
`action:group`) no longer forward `execute` into the runner, and Studio's
38+
`ActionPreview` no longer falls back to it — previewing an alias-only draft as
39+
"bound" contradicted the parse that rejects it on save.
40+
41+
Requires `@objectstack/spec` 17. Metadata still on the alias is rewritten by
42+
`os migrate meta --from 16`.

packages/app-shell/src/views/metadata-admin/previews/ActionPreview.tsx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,10 @@ export function ActionPreview({ name, draft }: MetadataPreviewProps) {
151151
const label = localize(d.label) || actionName;
152152
const icon = (d.icon as string | undefined) || undefined;
153153
const type = String(d.type ?? 'script');
154-
const target = (d.target as string | undefined) ?? (d.execute as string | undefined);
154+
// `target` only: the `execute` alias was removed in @objectstack/spec 17
155+
// (#3855, #3856). Reading it here would preview a draft as bound when the
156+
// spec rejects it at save, which is the opposite of what a preview is for.
157+
const target = d.target as string | undefined;
155158
const variant = (d.variant as string | undefined) || undefined;
156159
const component = String(d.component ?? '');
157160
const locations = Array.isArray(d.locations) ? (d.locations as string[]) : [];

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,6 @@ const ActionButtonRenderer = forwardRef<HTMLButtonElement, ActionButtonProps>(
9999
// Static "open in new tab" switch for url actions — forward so the
100100
// runner's executeUrl honors it (dropped, the toggle is silently lost).
101101
openIn: (schema as any).openIn,
102-
execute: schema.execute,
103102
endpoint: schema.endpoint,
104103
method: schema.method,
105104
...paramsPayload,

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,6 @@ const ActionGroupRenderer = forwardRef<HTMLDivElement, { schema: ActionGroupSche
200200
name: action.name,
201201
target: action.target,
202202
openIn: (action as any).openIn,
203-
execute: action.execute,
204203
endpoint: action.endpoint,
205204
method: action.method,
206205
params: action.params as Record<string, any> | undefined,

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,6 @@ const ActionIconRenderer = forwardRef<HTMLButtonElement, ActionIconProps>(
6262
name: schema.name,
6363
target: schema.target,
6464
openIn: (schema as any).openIn,
65-
execute: schema.execute,
6665
endpoint: schema.endpoint,
6766
method: schema.method,
6867
params: schema.params as Record<string, any> | undefined,

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,6 @@ const ActionMenuRenderer = forwardRef<HTMLButtonElement, { schema: ActionMenuSch
145145
name: action.name,
146146
target: action.target,
147147
openIn: (action as any).openIn,
148-
execute: action.execute,
149148
endpoint: action.endpoint,
150149
method: action.method,
151150
params: action.params as Record<string, any> | undefined,

packages/core/src/actions/ActionRunner.ts

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -128,9 +128,15 @@ export interface ActionDef {
128128
* reveal dialog rendering `result.data` (see ResultDialogSpec).
129129
*/
130130
resultDialog?: ResultDialogSpec;
131-
/** Script/expression to execute (for type: 'script') */
132-
execute?: string;
133-
/** Target URL or identifier (for type: 'url', 'modal', 'flow') */
131+
/**
132+
* The one handler slot: script name/expression for `type: 'script'`, URL for
133+
* `'url'`, flow name for `'flow'`, modal/page for `'modal'`, endpoint for
134+
* `'api'`, FormView name for `'form'`.
135+
*
136+
* The `execute` alias was REMOVED in @objectstack/spec 17 (#3855) and is not
137+
* read here (#3856) — don't re-add it. Two handler slots is how one action ran
138+
* one script server-side and a different one client-side (#3713).
139+
*/
134140
target?: string;
135141
/**
136142
* Action body (spec `ActionSchema.body` — `HookBodySchema`). Opaque here:
@@ -718,18 +724,17 @@ export class ActionRunner {
718724
* Supports ${} template expressions referencing data, record, user context.
719725
*/
720726
private async executeScript(action: ActionDef): Promise<ActionResult> {
721-
// `target` is the canonical binding; `execute` is its deprecated alias
722-
// (@objectstack/spec ActionSchema). Canonical wins when both are present,
723-
// matching the spec's own fold and ActionPreview's `target ?? execute`.
724-
// Spec >=16.1 folds `execute` into `target` and drops it at parse, so this
725-
// only bites on raw, unparsed metadata — where the two readers used to
726-
// disagree. Alias-only authoring still works via the fallback.
727-
const script = action.target || action.execute;
727+
// `target` is the only handler slot. The `execute` alias was removed in
728+
// @objectstack/spec 17 (#3855), which rejects an authored `execute` at parse
729+
// with the rename prescription — so parsed metadata cannot carry it and a
730+
// `target || execute` fallback could only ever evaluate to `target` (#3856).
731+
const script = action.target;
728732
if (!script) {
729733
// A spec `body` IS a script — this runner just cannot run one (see the
730734
// `body` field docs). Saying "no script provided" would send the author
731735
// hunting for a missing field they actually wrote, so name the real
732-
// cause and the remedy instead.
736+
// cause and the remedy instead. Checked before the retired `execute` key:
737+
// a `body` action ignores `target`, so "rename it" would be wrong advice.
733738
if (action.body != null) {
734739
return {
735740
success: false,
@@ -739,6 +744,20 @@ export class ActionRunner {
739744
'Register a `script` handler that POSTs to /api/v1/actions/{object}/{action}.',
740745
};
741746
}
747+
// ActionDef is open-ended (`[key: string]: any`), so hand-authored
748+
// metadata that never passed through the spec parser still compiles with
749+
// the retired key. Carry the same prescription the spec's tombstone does,
750+
// for the same reason: a bare "no script provided" reads as "you forgot a
751+
// field" to an author who did write one.
752+
if (typeof action.execute === 'string') {
753+
return {
754+
success: false,
755+
error:
756+
'`execute` was removed in @objectstack/spec 17 — rename the key to `target`. ' +
757+
'The value (a script name or expression) is unchanged. ' +
758+
'Run `os migrate meta --from 16` to rewrite it automatically.',
759+
};
760+
}
742761
return { success: false, error: 'No script provided for script action' };
743762
}
744763

packages/core/src/actions/__tests__/ActionEngine.test.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ describe('ActionEngine', () => {
9696

9797
describe('dispatch', () => {
9898
it('executes mapped actions for an event', async () => {
99-
engine.registerAction({ name: 'log', type: 'script', execute: '"logged"' });
99+
engine.registerAction({ name: 'log', type: 'script', target: '"logged"' });
100100
engine.addMapping({ event: 'row:click', actionName: 'log' });
101101

102102
const results = await engine.dispatch('row:click');
@@ -111,7 +111,7 @@ describe('ActionEngine', () => {
111111

112112
it('skips actions when mapping condition is false', async () => {
113113
engine = new ActionEngine({ data: { status: 'locked' } });
114-
engine.registerAction({ name: 'edit', type: 'script', execute: '"edited"' });
114+
engine.registerAction({ name: 'edit', type: 'script', target: '"edited"' });
115115
engine.addMapping({
116116
event: 'row:click',
117117
actionName: 'edit',
@@ -126,7 +126,7 @@ describe('ActionEngine', () => {
126126
describe('handleShortcut', () => {
127127
it('executes action for matching shortcut', async () => {
128128
engine.registerAction(
129-
{ name: 'save', type: 'script', execute: '"saved"' },
129+
{ name: 'save', type: 'script', target: '"saved"' },
130130
{ shortcut: 'ctrl+s' }
131131
);
132132

@@ -137,7 +137,7 @@ describe('ActionEngine', () => {
137137

138138
it('normalizes shortcut key order', async () => {
139139
engine.registerAction(
140-
{ name: 'save', type: 'script', execute: '"saved"' },
140+
{ name: 'save', type: 'script', target: '"saved"' },
141141
{ shortcut: 'ctrl+shift+s' }
142142
);
143143

@@ -156,7 +156,7 @@ describe('ActionEngine', () => {
156156
describe('executeBulk', () => {
157157
it('executes action on multiple records sequentially', async () => {
158158
engine.registerAction(
159-
{ name: 'approve', type: 'script', execute: '"approved"' },
159+
{ name: 'approve', type: 'script', target: '"approved"' },
160160
{ bulkEnabled: true }
161161
);
162162

packages/core/src/actions/__tests__/ActionRunner.scriptAwait.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ describe('ActionRunner - script action awaits a Promise-returning formula', () =
3232
// so the expression must call it by that same upper-cased name.
3333
runner.getEvaluator().registerFunction('doWrite', () => write);
3434

35-
const action: ActionDef = { type: 'script', execute: 'DOWRITE()' };
35+
const action: ActionDef = { type: 'script', target: 'DOWRITE()' };
3636
const pending = runner.execute(action);
3737

3838
// The write hasn't resolved yet — no toast should have fired.
@@ -49,7 +49,7 @@ describe('ActionRunner - script action awaits a Promise-returning formula', () =
4949

5050
it('surfaces a rejected write as success:false instead of a false-positive success toast', async () => {
5151
runner.getEvaluator().registerFunction('doFailingWrite', () => Promise.reject(new Error('write failed')));
52-
const action: ActionDef = { type: 'script', execute: 'DOFAILINGWRITE()' };
52+
const action: ActionDef = { type: 'script', target: 'DOFAILINGWRITE()' };
5353

5454
const result = await runner.execute(action);
5555

packages/core/src/actions/__tests__/ActionRunner.test.ts

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -194,13 +194,13 @@ describe('ActionRunner', () => {
194194
it('should evaluate script expression', async () => {
195195
const result = await runner.execute({
196196
type: 'script',
197-
execute: 'record.id + 100',
197+
target: 'record.id + 100',
198198
});
199199
expect(result.success).toBe(true);
200200
expect(result.data).toBe(101);
201201
});
202202

203-
it('should evaluate script from the canonical target field', async () => {
203+
it('should evaluate a script expression against the data scope', async () => {
204204
const result = await runner.execute({
205205
type: 'script',
206206
target: 'data.name',
@@ -209,17 +209,21 @@ describe('ActionRunner', () => {
209209
expect(result.data).toBe('Test');
210210
});
211211

212-
it('should prefer canonical target over the deprecated execute alias', async () => {
213-
// Spec >=16.1 folds `execute` into `target` at parse, so the two keys only
214-
// coexist on raw metadata. When they do, canonical wins — the same
215-
// precedence ActionPreview and the spec's own fold already use.
212+
it('should not read the retired execute alias, and should prescribe the rename', async () => {
213+
// `execute` was removed in @objectstack/spec 17 (#3855) — the parser now
214+
// rejects it outright, so no parsed action can carry it and the runner has
215+
// exactly one handler slot (#3856). Pinned as a test because the failure
216+
// mode of re-adding `target || execute` is invisible: it type-checks (
217+
// ActionDef is open-ended) and it runs, it just resurrects the two-slot
218+
// ambiguity that had one action running different scripts on each side of
219+
// the wire (#3713).
216220
const result = await runner.execute({
217221
type: 'script',
218-
target: 'data.name',
219222
execute: 'record.id + 100',
220223
});
221-
expect(result.success).toBe(true);
222-
expect(result.data).toBe('Test');
224+
expect(result.success).toBe(false);
225+
expect(result.error).toContain('`execute` was removed');
226+
expect(result.error).toContain('`target`');
223227
});
224228

225229
it('should fail when no script provided', async () => {
@@ -264,7 +268,7 @@ describe('ActionRunner', () => {
264268
it('should return data as undefined for expressions referencing missing vars', async () => {
265269
const result = await runner.execute({
266270
type: 'script',
267-
execute: 'data.nonExistent',
271+
target: 'data.nonExistent',
268272
});
269273
// ExpressionEvaluator returns undefined for missing properties (doesn't throw)
270274
expect(result.success).toBe(true);
@@ -875,7 +879,7 @@ describe('ActionRunner', () => {
875879
describe('executeAction', () => {
876880
it('should execute an action with the convenience function', async () => {
877881
const result = await executeAction(
878-
{ type: 'script', execute: '1 + 2' },
882+
{ type: 'script', target: '1 + 2' },
879883
{ data: {} },
880884
);
881885
expect(result.success).toBe(true);

0 commit comments

Comments
 (0)