From 924fb83a27fefa6d0c4936c7402b625d204b919c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 07:39:52 +0000 Subject: [PATCH] =?UTF-8?q?refactor(actions)!:=20`target`=20is=20the=20onl?= =?UTF-8?q?y=20handler=20slot=20=E2=80=94=20delete=20the=20`execute`=20ali?= =?UTF-8?q?as=20(objectstack#3856)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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`. Verified against 17.0.0-rc.0 — an action declaring `execute` fails `ActionSchema.safeParse` and a `target` action's parsed output has no `execute` key. 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`). A dead slot still reads as a live contract. 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 (previewing an alias-only draft as bound contradicted the parse that rejects it on save). Tests move to `target`; the "prefer canonical over the alias" case becomes a regression test that the alias is not read. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014DBMo8gnFduzgpuSHd5Eje --- .../action-target-is-the-only-handler-slot.md | 42 +++++++++++++++++++ .../metadata-admin/previews/ActionPreview.tsx | 5 ++- .../src/renderers/action/action-button.tsx | 1 - .../src/renderers/action/action-group.tsx | 1 - .../src/renderers/action/action-icon.tsx | 1 - .../src/renderers/action/action-menu.tsx | 1 - packages/core/src/actions/ActionRunner.ts | 41 +++++++++++++----- .../actions/__tests__/ActionEngine.test.ts | 10 ++--- .../ActionRunner.scriptAwait.test.ts | 4 +- .../actions/__tests__/ActionRunner.test.ts | 26 +++++++----- .../context/__tests__/ActionContext.test.tsx | 8 ++-- .../useActionEngine.sharedRunner.test.tsx | 6 +-- .../hooks/__tests__/useActionEngine.test.ts | 4 +- packages/types/src/ui-action.ts | 18 ++++---- 14 files changed, 118 insertions(+), 50 deletions(-) create mode 100644 .changeset/action-target-is-the-only-handler-slot.md diff --git a/.changeset/action-target-is-the-only-handler-slot.md b/.changeset/action-target-is-the-only-handler-slot.md new file mode 100644 index 0000000000..7d5989ec72 --- /dev/null +++ b/.changeset/action-target-is-the-only-handler-slot.md @@ -0,0 +1,42 @@ +--- +'@object-ui/types': major +'@object-ui/core': major +'@object-ui/components': major +'@object-ui/app-shell': major +--- + +**`target` is the only action handler slot — the `execute` alias is gone from the renderer (framework#3856).** + +`ActionRunner.executeScript` read `action.target || action.execute`. That fallback +is unreachable against `@objectstack/spec` 17: `execute` is now a tombstoned key +(framework#3855) that the parser **rejects** with the rename prescription, so no +parsed action can carry it and the `||` could only ever yield `target`. Verified +against 17.0.0-rc.0 — an action declaring `execute` fails `ActionSchema.safeParse`, +and a `target` action's parsed output has no `execute` key at all. + +Deleted rather than left as harmless residue: two handler slots is what let one +action run one script server-side and a different one client-side (framework#3713, +where this renderer preferred the alias while the spec transform preferred +`target`). A dead slot still reads as a live contract to the next maintainer. + +`execute` is also **removed from the types**, which is the part that had never +landed. framework#3856 predicted a compile error here; there wasn't one, because +neither reader was typed against the spec's `z.infer`: + +- `@object-ui/types` `ActionSchema` hand-declared `execute?: string`. Removed, so + `execute: '…'` now fails `tsc` at the authoring site (TS2353). +- `@object-ui/core` `ActionDef` hand-declared it too. Removed — but `ActionDef` + carries a `[key: string]: any` index signature, so stale hand-authored metadata + that never passed through the parser still compiles. For that path + `executeScript` now returns the rename prescription instead of a bare + "No script provided", matching the spec tombstone's rule that removing an + authorable key must be audible: silently binding no handler is the + "Mark Done does nothing" shape (framework#2169). + +The four action renderers (`action:button`, `action:icon`, `action:menu`, +`action:group`) no longer forward `execute` into the runner, and Studio's +`ActionPreview` no longer falls back to it — previewing an alias-only draft as +"bound" contradicted the parse that rejects it on save. + +Requires `@objectstack/spec` 17. Metadata still on the alias is rewritten by +`os migrate meta --from 16`. diff --git a/packages/app-shell/src/views/metadata-admin/previews/ActionPreview.tsx b/packages/app-shell/src/views/metadata-admin/previews/ActionPreview.tsx index 5475b3e192..45c4fdf4a1 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/ActionPreview.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/ActionPreview.tsx @@ -151,7 +151,10 @@ export function ActionPreview({ name, draft }: MetadataPreviewProps) { const label = localize(d.label) || actionName; const icon = (d.icon as string | undefined) || undefined; const type = String(d.type ?? 'script'); - const target = (d.target as string | undefined) ?? (d.execute as string | undefined); + // `target` only: the `execute` alias was removed in @objectstack/spec 17 + // (#3855, #3856). Reading it here would preview a draft as bound when the + // spec rejects it at save, which is the opposite of what a preview is for. + const target = d.target as string | undefined; const variant = (d.variant as string | undefined) || undefined; const component = String(d.component ?? ''); const locations = Array.isArray(d.locations) ? (d.locations as string[]) : []; diff --git a/packages/components/src/renderers/action/action-button.tsx b/packages/components/src/renderers/action/action-button.tsx index 3e49281b31..1687477646 100644 --- a/packages/components/src/renderers/action/action-button.tsx +++ b/packages/components/src/renderers/action/action-button.tsx @@ -99,7 +99,6 @@ const ActionButtonRenderer = forwardRef( // Static "open in new tab" switch for url actions — forward so the // runner's executeUrl honors it (dropped, the toggle is silently lost). openIn: (schema as any).openIn, - execute: schema.execute, endpoint: schema.endpoint, method: schema.method, ...paramsPayload, diff --git a/packages/components/src/renderers/action/action-group.tsx b/packages/components/src/renderers/action/action-group.tsx index f9b1b148f7..b4ad374755 100644 --- a/packages/components/src/renderers/action/action-group.tsx +++ b/packages/components/src/renderers/action/action-group.tsx @@ -200,7 +200,6 @@ const ActionGroupRenderer = forwardRef | undefined, diff --git a/packages/components/src/renderers/action/action-icon.tsx b/packages/components/src/renderers/action/action-icon.tsx index 0b5ac7e1f0..a2fd48b2c0 100644 --- a/packages/components/src/renderers/action/action-icon.tsx +++ b/packages/components/src/renderers/action/action-icon.tsx @@ -62,7 +62,6 @@ const ActionIconRenderer = forwardRef( name: schema.name, target: schema.target, openIn: (schema as any).openIn, - execute: schema.execute, endpoint: schema.endpoint, method: schema.method, params: schema.params as Record | undefined, diff --git a/packages/components/src/renderers/action/action-menu.tsx b/packages/components/src/renderers/action/action-menu.tsx index abd00ba9e3..a3fc9b6149 100644 --- a/packages/components/src/renderers/action/action-menu.tsx +++ b/packages/components/src/renderers/action/action-menu.tsx @@ -145,7 +145,6 @@ const ActionMenuRenderer = forwardRef | undefined, diff --git a/packages/core/src/actions/ActionRunner.ts b/packages/core/src/actions/ActionRunner.ts index 205ed35a77..4c36380a80 100644 --- a/packages/core/src/actions/ActionRunner.ts +++ b/packages/core/src/actions/ActionRunner.ts @@ -128,9 +128,15 @@ export interface ActionDef { * reveal dialog rendering `result.data` (see ResultDialogSpec). */ resultDialog?: ResultDialogSpec; - /** Script/expression to execute (for type: 'script') */ - execute?: string; - /** Target URL or identifier (for type: 'url', 'modal', 'flow') */ + /** + * The one handler slot: script name/expression for `type: 'script'`, URL for + * `'url'`, flow name for `'flow'`, modal/page for `'modal'`, endpoint for + * `'api'`, FormView name for `'form'`. + * + * The `execute` alias was REMOVED in @objectstack/spec 17 (#3855) and is not + * read here (#3856) — don't re-add it. Two handler slots is how one action ran + * one script server-side and a different one client-side (#3713). + */ target?: string; /** * Action body (spec `ActionSchema.body` — `HookBodySchema`). Opaque here: @@ -718,18 +724,17 @@ export class ActionRunner { * Supports ${} template expressions referencing data, record, user context. */ private async executeScript(action: ActionDef): Promise { - // `target` is the canonical binding; `execute` is its deprecated alias - // (@objectstack/spec ActionSchema). Canonical wins when both are present, - // matching the spec's own fold and ActionPreview's `target ?? execute`. - // Spec >=16.1 folds `execute` into `target` and drops it at parse, so this - // only bites on raw, unparsed metadata — where the two readers used to - // disagree. Alias-only authoring still works via the fallback. - const script = action.target || action.execute; + // `target` is the only handler slot. The `execute` alias was removed in + // @objectstack/spec 17 (#3855), which rejects an authored `execute` at parse + // with the rename prescription — so parsed metadata cannot carry it and a + // `target || execute` fallback could only ever evaluate to `target` (#3856). + const script = action.target; if (!script) { // A spec `body` IS a script — this runner just cannot run one (see the // `body` field docs). Saying "no script provided" would send the author // hunting for a missing field they actually wrote, so name the real - // cause and the remedy instead. + // cause and the remedy instead. Checked before the retired `execute` key: + // a `body` action ignores `target`, so "rename it" would be wrong advice. if (action.body != null) { return { success: false, @@ -739,6 +744,20 @@ export class ActionRunner { 'Register a `script` handler that POSTs to /api/v1/actions/{object}/{action}.', }; } + // ActionDef is open-ended (`[key: string]: any`), so hand-authored + // metadata that never passed through the spec parser still compiles with + // the retired key. Carry the same prescription the spec's tombstone does, + // for the same reason: a bare "no script provided" reads as "you forgot a + // field" to an author who did write one. + if (typeof action.execute === 'string') { + return { + success: false, + error: + '`execute` was removed in @objectstack/spec 17 — rename the key to `target`. ' + + 'The value (a script name or expression) is unchanged. ' + + 'Run `os migrate meta --from 16` to rewrite it automatically.', + }; + } return { success: false, error: 'No script provided for script action' }; } diff --git a/packages/core/src/actions/__tests__/ActionEngine.test.ts b/packages/core/src/actions/__tests__/ActionEngine.test.ts index c5063b258e..b5bb548f4c 100644 --- a/packages/core/src/actions/__tests__/ActionEngine.test.ts +++ b/packages/core/src/actions/__tests__/ActionEngine.test.ts @@ -96,7 +96,7 @@ describe('ActionEngine', () => { describe('dispatch', () => { it('executes mapped actions for an event', async () => { - engine.registerAction({ name: 'log', type: 'script', execute: '"logged"' }); + engine.registerAction({ name: 'log', type: 'script', target: '"logged"' }); engine.addMapping({ event: 'row:click', actionName: 'log' }); const results = await engine.dispatch('row:click'); @@ -111,7 +111,7 @@ describe('ActionEngine', () => { it('skips actions when mapping condition is false', async () => { engine = new ActionEngine({ data: { status: 'locked' } }); - engine.registerAction({ name: 'edit', type: 'script', execute: '"edited"' }); + engine.registerAction({ name: 'edit', type: 'script', target: '"edited"' }); engine.addMapping({ event: 'row:click', actionName: 'edit', @@ -126,7 +126,7 @@ describe('ActionEngine', () => { describe('handleShortcut', () => { it('executes action for matching shortcut', async () => { engine.registerAction( - { name: 'save', type: 'script', execute: '"saved"' }, + { name: 'save', type: 'script', target: '"saved"' }, { shortcut: 'ctrl+s' } ); @@ -137,7 +137,7 @@ describe('ActionEngine', () => { it('normalizes shortcut key order', async () => { engine.registerAction( - { name: 'save', type: 'script', execute: '"saved"' }, + { name: 'save', type: 'script', target: '"saved"' }, { shortcut: 'ctrl+shift+s' } ); @@ -156,7 +156,7 @@ describe('ActionEngine', () => { describe('executeBulk', () => { it('executes action on multiple records sequentially', async () => { engine.registerAction( - { name: 'approve', type: 'script', execute: '"approved"' }, + { name: 'approve', type: 'script', target: '"approved"' }, { bulkEnabled: true } ); diff --git a/packages/core/src/actions/__tests__/ActionRunner.scriptAwait.test.ts b/packages/core/src/actions/__tests__/ActionRunner.scriptAwait.test.ts index 31eadeba4c..760e7c053e 100644 --- a/packages/core/src/actions/__tests__/ActionRunner.scriptAwait.test.ts +++ b/packages/core/src/actions/__tests__/ActionRunner.scriptAwait.test.ts @@ -32,7 +32,7 @@ describe('ActionRunner - script action awaits a Promise-returning formula', () = // so the expression must call it by that same upper-cased name. runner.getEvaluator().registerFunction('doWrite', () => write); - const action: ActionDef = { type: 'script', execute: 'DOWRITE()' }; + const action: ActionDef = { type: 'script', target: 'DOWRITE()' }; const pending = runner.execute(action); // The write hasn't resolved yet — no toast should have fired. @@ -49,7 +49,7 @@ describe('ActionRunner - script action awaits a Promise-returning formula', () = it('surfaces a rejected write as success:false instead of a false-positive success toast', async () => { runner.getEvaluator().registerFunction('doFailingWrite', () => Promise.reject(new Error('write failed'))); - const action: ActionDef = { type: 'script', execute: 'DOFAILINGWRITE()' }; + const action: ActionDef = { type: 'script', target: 'DOFAILINGWRITE()' }; const result = await runner.execute(action); diff --git a/packages/core/src/actions/__tests__/ActionRunner.test.ts b/packages/core/src/actions/__tests__/ActionRunner.test.ts index b16ec661f1..3ed80465ca 100644 --- a/packages/core/src/actions/__tests__/ActionRunner.test.ts +++ b/packages/core/src/actions/__tests__/ActionRunner.test.ts @@ -194,13 +194,13 @@ describe('ActionRunner', () => { it('should evaluate script expression', async () => { const result = await runner.execute({ type: 'script', - execute: 'record.id + 100', + target: 'record.id + 100', }); expect(result.success).toBe(true); expect(result.data).toBe(101); }); - it('should evaluate script from the canonical target field', async () => { + it('should evaluate a script expression against the data scope', async () => { const result = await runner.execute({ type: 'script', target: 'data.name', @@ -209,17 +209,21 @@ describe('ActionRunner', () => { expect(result.data).toBe('Test'); }); - it('should prefer canonical target over the deprecated execute alias', async () => { - // Spec >=16.1 folds `execute` into `target` at parse, so the two keys only - // coexist on raw metadata. When they do, canonical wins — the same - // precedence ActionPreview and the spec's own fold already use. + it('should not read the retired execute alias, and should prescribe the rename', async () => { + // `execute` was removed in @objectstack/spec 17 (#3855) — the parser now + // rejects it outright, so no parsed action can carry it and the runner has + // exactly one handler slot (#3856). Pinned as a test because the failure + // mode of re-adding `target || execute` is invisible: it type-checks ( + // ActionDef is open-ended) and it runs, it just resurrects the two-slot + // ambiguity that had one action running different scripts on each side of + // the wire (#3713). const result = await runner.execute({ type: 'script', - target: 'data.name', execute: 'record.id + 100', }); - expect(result.success).toBe(true); - expect(result.data).toBe('Test'); + expect(result.success).toBe(false); + expect(result.error).toContain('`execute` was removed'); + expect(result.error).toContain('`target`'); }); it('should fail when no script provided', async () => { @@ -264,7 +268,7 @@ describe('ActionRunner', () => { it('should return data as undefined for expressions referencing missing vars', async () => { const result = await runner.execute({ type: 'script', - execute: 'data.nonExistent', + target: 'data.nonExistent', }); // ExpressionEvaluator returns undefined for missing properties (doesn't throw) expect(result.success).toBe(true); @@ -875,7 +879,7 @@ describe('ActionRunner', () => { describe('executeAction', () => { it('should execute an action with the convenience function', async () => { const result = await executeAction( - { type: 'script', execute: '1 + 2' }, + { type: 'script', target: '1 + 2' }, { data: {} }, ); expect(result.success).toBe(true); diff --git a/packages/react/src/context/__tests__/ActionContext.test.tsx b/packages/react/src/context/__tests__/ActionContext.test.tsx index aa05db73be..698d77df38 100644 --- a/packages/react/src/context/__tests__/ActionContext.test.tsx +++ b/packages/react/src/context/__tests__/ActionContext.test.tsx @@ -66,7 +66,7 @@ describe('ActionContext', () => { await act(async () => { actionResult = await result.current.execute({ type: 'script', - execute: 'true', + target: 'true', toast: { showOnSuccess: true }, successMessage: 'Done!', }); @@ -117,8 +117,8 @@ describe('ActionContext', () => { let chainResult: any; await act(async () => { chainResult = await result.current.executeChain([ - { type: 'script', execute: 'true' }, - { type: 'script', execute: 'true' }, + { type: 'script', target: 'true' }, + { type: 'script', target: 'true' }, ]); }); @@ -162,7 +162,7 @@ describe('ActionContext', () => { await act(async () => { await result.current.execute({ type: 'script', - execute: 'true', + target: 'true', toast: { showOnSuccess: true }, successMessage: 'Saved!', }); diff --git a/packages/react/src/hooks/__tests__/useActionEngine.sharedRunner.test.tsx b/packages/react/src/hooks/__tests__/useActionEngine.sharedRunner.test.tsx index af19fbe5a8..9eac613a0e 100644 --- a/packages/react/src/hooks/__tests__/useActionEngine.sharedRunner.test.tsx +++ b/packages/react/src/hooks/__tests__/useActionEngine.sharedRunner.test.tsx @@ -80,7 +80,7 @@ describe('useActionEngine — shared ActionProvider runner', () => { { name: 'change_password', type: 'script', - execute: 'true', + target: 'true', visible: 'record.id == ctx.user.id', locations: ['record_section'], } as any, @@ -114,14 +114,14 @@ describe('useActionEngine — shared ActionProvider runner', () => { { name: 'self_only_flat', type: 'script', - execute: 'true', + target: 'true', visible: 'record.id == user.id', locations: ['record_section'], } as any, { name: 'self_only_ctx', type: 'script', - execute: 'true', + target: 'true', visible: 'record.id == ctx.user.id', locations: ['record_section'], } as any, diff --git a/packages/react/src/hooks/__tests__/useActionEngine.test.ts b/packages/react/src/hooks/__tests__/useActionEngine.test.ts index f18d816f5e..4ab1c15bd1 100644 --- a/packages/react/src/hooks/__tests__/useActionEngine.test.ts +++ b/packages/react/src/hooks/__tests__/useActionEngine.test.ts @@ -11,7 +11,7 @@ describe('useActionEngine', () => { { name: 'mark_complete', type: 'script', - execute: 'true', + target: 'true', locations: ['list_toolbar', 'record_header'], bulkEnabled: true, }, @@ -30,7 +30,7 @@ describe('useActionEngine', () => { { name: 'global_search', type: 'script', - execute: 'true', + target: 'true', locations: ['global_nav'], shortcut: 'ctrl+k', }, diff --git a/packages/types/src/ui-action.ts b/packages/types/src/ui-action.ts index 1cc1dae59f..b97d727209 100644 --- a/packages/types/src/ui-action.ts +++ b/packages/types/src/ui-action.ts @@ -173,7 +173,14 @@ export interface ActionSchema { /** Action execution type */ type: ActionType; - /** Target for the action (URL, script name, etc.) */ + /** + * Target for the action (URL, script name, etc.) — the **only** handler slot. + * + * The `execute` alias was removed in `@objectstack/spec` 17 (#3855); this + * interface no longer declares it, so `execute: '…'` now fails `tsc` at the + * authoring site instead of binding a second handler nothing agrees on + * (#3713, #3856). Rename to `target`; the value is unchanged. + */ target?: string; /** @@ -197,9 +204,6 @@ export interface ActionSchema { */ openIn?: 'self' | 'new-tab'; - /** Script to execute (for type: 'script') */ - execute?: string; - /** API endpoint (for type: 'api') */ endpoint?: string; @@ -283,9 +287,9 @@ export interface ActionSchema { * URL to the clipboard — UI side-effects that are not part of the domain * action protocol and therefore need not be serialized over the wire. * - * When present, `onClick` takes precedence over `type` / `target` / - * `execute`. Prefer {@link ActionEngine}-routed actions for anything that - * could originate from server-driven metadata. + * When present, `onClick` takes precedence over `type` / `target`. Prefer + * {@link ActionEngine}-routed actions for anything that could originate from + * server-driven metadata. */ onClick?: () => void | Promise; }