diff --git a/.changeset/13022-action-row-declared-patch.md b/.changeset/13022-action-row-declared-patch.md new file mode 100644 index 0000000000..3745257df8 --- /dev/null +++ b/.changeset/13022-action-row-declared-patch.md @@ -0,0 +1,16 @@ +--- +'@object-ui/core': patch +'@object-ui/components': patch +'@object-ui/app-shell': patch +--- + +The Undo of an `operation: 'update'` action covers the `patch` of the declared action its `name` names, and no action renderer forwards a page action row's own `patch` (objectui#13022, following `@objectstack/spec` 18 retiring that row key, objectstack#24442). + +A page action row (`action:button`, `action:icon`, an `action:group` / `action:menu` member) with `operation: 'update'` runs the declared action its `name` names. The platform action route resolves that declaration by name and writes its `patch` under the collected params. The row's own `patch` reached no request since objectui#12941, and the runner read it only for the Undo snapshot, so the snapshot covered the wrong fields. A row with no `patch` of its own snapshotted the collected params alone, not the fields the route wrote. + +- **Undo restores the fields the route writes.** `ActionRunner` reads the `patch` of the declaration the action's `name` names from the `objectActions` its host publishes beside `objectName`, plus the collected params. A declaration with no `patch` keeps the params-only snapshot. A written field the row does not carry still offers no Undo. +- **The console publishes the declarations.** `useConsoleActionRuntime` and the record page publish the object's `actions` as `objectActions` in the runner context, beside `objectFields`. +- **A host that publishes none** is unchanged for an action it dispatches whole (a grid row, a declared-actions bar, a related-list row): the runner reads that def's own `patch`, which is the declaration's. +- **`action:button`, `action:icon`, `action:group` and `action:menu` no longer forward `patch`.** The request body is unchanged: it carries the collected params alone, as since objectui#12941. + +The `patch` input `action:button` and `action:icon` publish is unchanged here. It follows the spec pin. diff --git a/content/docs/guide/layout.md b/content/docs/guide/layout.md index 48312a4406..9df55f7676 100644 --- a/content/docs/guide/layout.md +++ b/content/docs/guide/layout.md @@ -681,16 +681,21 @@ The rest of the action is authored on the same node. `action:button` and `action evaluate `visible` and `disabled` themselves (a boolean, a CEL string, or a `{ dialect, source }` envelope), and forward the action-definition keys to the runner: `params`, `confirmText`, `successMessage`, `errorMessage`, `toast`, `bodyExtra`, -`bodyShape`, `operation` / `patch`, `onSuccess`, `objectName` and the others each +`bodyShape`, `operation`, `onSuccess`, `objectName` and the others each registration's `inputs` lists, with a `description` per key. For an `api` action, write the request URL in `target`: `endpoint` is not published, because the console's `api` handler never reads it. +An `operation: update` node runs the declared action its `name` names: the platform +action route writes that declaration's `patch` under the collected params. A `patch` +written on the node itself is not forwarded and writes nothing. + `undoable: true` on an `operation: update` makes the success toast offer Undo, which -writes back the prior values of the fields the update wrote. `action:button` reads those -values off the record in scope: the record page's record, or the row the host binds -through `data`. A button with no record in scope offers no Undo, because there is no row -to restore; neither does one whose `recordId` names a record other than the one in scope. +writes back the prior values of the fields the update wrote: the named declaration's +`patch` fields and the collected params. `action:button` reads those values off the +record in scope: the record page's record, or the row the host binds through `data`. A +button with no record in scope offers no Undo, because there is no row to restore; +neither does one whose `recordId` names a record other than the one in scope. ## Responsive Behavior diff --git a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx index 422d7ba286..90ff5842aa 100644 --- a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx +++ b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx @@ -788,6 +788,10 @@ export function useConsoleActionRuntime(opts: ConsoleActionRuntimeOptions): Cons // Undo capture reads them to tell a relation, captured as its stored id, // from a field whose value merely has an object's shape. ...(objectName && objectDef?.fields ? { objectFields: objectDef.fields } : {}), + // Its declared actions, beside them (objectui#13022): the Undo capture + // of an `operation: 'update'` reads the `patch` of the declaration the + // action's `name` names, the one the platform action route writes. + ...(objectName && Array.isArray(objectDef?.actions) ? { objectActions: objectDef.actions } : {}), user: currentUser, // Backend origin — lets `type: 'url'` actions issue full-page // navigations to API endpoints across origins in dev. diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index 9d8c2e03e6..8a61f344d2 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/packages/app-shell/src/views/RecordDetailView.tsx @@ -3676,6 +3676,10 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri // (objectui#11122): the runner's `operation: 'update'` Undo capture // reads them to capture a relation `$expand` filled as its stored id. ...(objectDef?.fields ? { objectFields: objectDef.fields } : {}), + // Its declared actions (objectui#13022): the same capture reads the + // `patch` of the declaration an update action's `name` names, + // which is what the platform action route writes. + ...(Array.isArray(objectDef?.actions) ? { objectActions: objectDef.actions } : {}), user: currentUser, org: actionContextOrg(activeOrganization), }} diff --git a/packages/components/src/renderers/action/__tests__/action-button-icon-inputs-11168.test.tsx b/packages/components/src/renderers/action/__tests__/action-button-icon-inputs-11168.test.tsx index b06d79dfa2..62652dabef 100644 --- a/packages/components/src/renderers/action/__tests__/action-button-icon-inputs-11168.test.tsx +++ b/packages/components/src/renderers/action/__tests__/action-button-icon-inputs-11168.test.tsx @@ -343,7 +343,7 @@ describe.each(BLOCKS)('%s — keys the runner acts on, forwarded by the block', expect(await bodyFor({})).toEqual({ name: 'Acme', source: 'web' }); }); - it('`operation: update` reaches the write route with its `patch`, which never rides in the supplied values', async () => { + it('`operation: update` reaches the write route; a `patch` on the node is not forwarded and never rides in the supplied values', async () => { const script = vi.fn(async () => ({ success: true })); mount( node(type, { @@ -356,11 +356,13 @@ describe.each(BLOCKS)('%s — keys the runner acts on, forwarded by the block', ); press(); await waitFor(() => expect(script).toHaveBeenCalledTimes(1)); - // The block forwarded `operation` and `patch`, so the update path ran. The - // route applies `patch` from the action's declaration, so the values the - // dispatch posts are the supplied ones alone (objectui#12941): no `stage`. + // The block forwarded `operation`, so the update path ran. The route writes + // the `patch` of the declaration the node's `name` names, never the node's + // own, so the block forwards no `patch` at all (objectui#13022), and the + // values the dispatch posts are the supplied ones alone (objectui#12941). const dispatched = (script.mock.calls[0] as unknown as [ActionDef])[0]; - expect(dispatched).toMatchObject({ operation: 'update', patch: { status: 'closed', stage: 'done' } }); + expect(dispatched).toMatchObject({ operation: 'update' }); + expect(Object.prototype.hasOwnProperty.call(dispatched, 'patch')).toBe(false); expect(dispatched.params).toEqual({ recordId: 'R1', status: 'user-chosen' }); cleanup(); script.mockClear(); diff --git a/packages/components/src/renderers/action/__tests__/action-button-undoable-11168.test.tsx b/packages/components/src/renderers/action/__tests__/action-button-undoable-11168.test.tsx index b98ac21f11..befdd91f6a 100644 --- a/packages/components/src/renderers/action/__tests__/action-button-undoable-11168.test.tsx +++ b/packages/components/src/renderers/action/__tests__/action-button-undoable-11168.test.tsx @@ -52,6 +52,18 @@ import '../action-bar'; /** The record the action runs on, as the page or the row carries it. */ const RECORD = { id: 't1', status: 'open', title: 'Write the report' }; +/** + * The object's declared actions, which the host publishes beside its object + * (`objectActions`, objectui#13022). The route writes the `patch` of the one a + * node's `name` names, so that is what the Undo snapshot covers; a node + * carries no `patch` of its own (`@objectstack/spec` 18 retires the row key). + */ +const DECLARATIONS = [ + { name: 'close_task', label: 'Close', operation: 'update', patch: { status: 'done' }, undoable: true }, + // A declaration with no `patch`: it writes the collected params alone. + { name: 'set_status', label: 'Set status', operation: 'update', undoable: true }, +]; + type Props = NonNullable['properties']>; /** One `action:button` node, its props in the `properties` bag as the spec places them. */ @@ -61,7 +73,6 @@ const button = (extra: Partial = {}): PublicBlockNodeOf<'action:button'> name: 'close_task', label: 'Close', operation: 'update', - patch: { status: 'done' }, undoable: true, ...extra, }, @@ -95,12 +106,16 @@ afterEach(() => { globalUndoManager.clear(); }); -/** The host's runner, publishing its object the way every console host does (`context.objectName`). */ +/** + * The host's runner, publishing its object the way every console host does + * (`context.objectName`), and that object's declarations beside it + * (`context.objectActions`). + */ function Runner({ children }: { children: React.ReactNode }) { return ( ({ status: 'blocked' })) as never} > @@ -167,9 +182,9 @@ describe('action:button `undoable` — an update of the record in scope offers U redoData: { status: 'done' }, }); // The write addressed that record, and the stash stayed on the client. The - // `patch` is the route's, applied from the declaration, so the request - // carries no field value at all (objectui#12941) while Undo still covers - // `status`, the field the route wrote. + // `patch` is the route's, applied from the declaration `close_task` names, + // so the request carries no field value at all (objectui#12941) while Undo + // covers `status`, the field the route wrote (objectui#13022). expect(sentBody()).toEqual({ recordId: 't1', params: {} }); }); @@ -207,8 +222,9 @@ describe('action:button `undoable` — an update of the record in scope offers U it('a value the user supplies is restored too, from the same record', async () => { // The input list collects `status`; the collected value is written, so its - // prior value is what Undo restores. - onRecordPage(button({ patch: undefined, params: [{ name: 'status', type: 'text', label: 'Status' }] })); + // prior value is what Undo restores. `set_status` declares no `patch`, so + // the collected value is the whole write. + onRecordPage(button({ name: 'set_status', params: [{ name: 'status', type: 'text', label: 'Status' }] })); await press(); expect(globalUndoManager.peekUndo()).toMatchObject({ undoData: { status: 'open' }, @@ -270,7 +286,7 @@ describe('action:button `undoable` — only an `undoable` update is handed the r }); it('an `undoable` action that is not an update carries no record', async () => { - onRecordPage(button({ operation: undefined, patch: undefined, actionType: 'script' })); + onRecordPage(button({ operation: undefined, actionType: 'script' })); await press(); expect(dispatched).toHaveLength(1); expect(carriedStash()).toBe(false); diff --git a/packages/components/src/renderers/action/__tests__/action-forward-precedence.test.tsx b/packages/components/src/renderers/action/__tests__/action-forward-precedence.test.tsx index 67441c5520..3cf78fd844 100644 --- a/packages/components/src/renderers/action/__tests__/action-forward-precedence.test.tsx +++ b/packages/components/src/renderers/action/__tests__/action-forward-precedence.test.tsx @@ -98,13 +98,17 @@ const CONTEXT = { target: 'contextWins', extraFromHost: 'present' }; * exactly the reading these two lists exist to give: the diff shows one added * name and no re-ordering of the keys the #4281 hoist was about. * - * `operation` / `patch` (objectui#7551) are the first pair added in the MIDDLE - * rather than at the tail — they sit with `bodyExtra` / `bodyShape` because - * they shape the same request. The reading is unchanged and is why the position - * was acceptable: the list is compared whole, so the failure diff still shows + * `operation` (objectui#7551) was the first key added in the MIDDLE rather + * than at the tail — it sits with `bodyExtra` / `bodyShape` because it shapes + * the same request. The reading is unchanged and is why the position was + * acceptable: the list is compared whole, so the failure diff still shows * exactly the added names against a sequence of pre-existing keys whose * relative order did not move. An insertion that DID reorder the neighbours - * would show up here as several moved names, not two added ones. + * would show up here as several moved names, not one added one. Its partner + * `patch` left both lists with objectui#13022: the route writes the `patch` of + * the declaration the action's `name` names, never a node's own, so no + * renderer forwards one. A removal reads the same way: one name gone, no + * neighbour moved. * * `outcomeMessages` (objectui#11344) is the next middle insertion, beside * `successMessage`, the copy it outranks in the success toast. Same reading: @@ -113,7 +117,7 @@ const CONTEXT = { target: 'contextWins', extraFromHost: 'present' }; const BUTTON_ORDER = [ 'type', 'name', 'label', 'description', 'target', 'openIn', 'endpoint', 'method', 'params', - 'bodyExtra', 'bodyShape', 'operation', 'patch', 'confirmText', 'successMessage', 'outcomeMessages', 'errorMessage', + 'bodyExtra', 'bodyShape', 'operation', 'confirmText', 'successMessage', 'outcomeMessages', 'errorMessage', 'refreshAfter', 'undoable', 'recordIdField', 'locations', 'toast', 'resultDialog', 'onSuccess', 'objectName', ]; @@ -125,7 +129,7 @@ const BUTTON_ORDER = [ const ICON_ORDER = [ 'type', 'name', 'label', 'description', 'target', 'openIn', 'endpoint', 'method', 'params', - 'bodyExtra', 'bodyShape', 'operation', 'patch', 'confirmText', 'successMessage', 'outcomeMessages', 'errorMessage', + 'bodyExtra', 'bodyShape', 'operation', 'confirmText', 'successMessage', 'outcomeMessages', 'errorMessage', 'refreshAfter', 'locations', 'toast', 'resultDialog', 'onSuccess', 'objectName', ]; diff --git a/packages/components/src/renderers/action/__tests__/action-row-declared-patch-13022.test.tsx b/packages/components/src/renderers/action/__tests__/action-row-declared-patch-13022.test.tsx new file mode 100644 index 0000000000..1fbac0936f --- /dev/null +++ b/packages/components/src/renderers/action/__tests__/action-row-declared-patch-13022.test.tsx @@ -0,0 +1,262 @@ +/** + * 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#13022 — a page action row's own `patch` is not what an update + * writes, so no action renderer forwards it, and the Undo snapshot covers the + * `patch` of the DECLARATION the row's `name` names. + * + * A row's `operation: 'update'` runs the declared action its `name` names: the + * platform action route resolves that declaration by name and writes ITS + * `patch` under the params it is sent. The row's own `patch` reached no + * request (objectui#12941), and `@objectstack/spec` 18 retires it on + * `action:button`, `action:icon` and the `action:group` / `action:menu` + * member. It was read for one thing only, the Undo snapshot, which therefore + * covered the wrong fields: a row with no `patch` of its own (every row, once + * the key is retired) snapshotted the collected params and not the fields the + * route wrote. + * + * Driven end to end through the real pieces: the real `SchemaRenderer` + * renders the real block, the click goes through the real `ActionRunner`, and + * the `script` dispatch is the real `createServerActionHandler` the console + * builds its route dispatch on, over a stubbed `fetch`. The host publishes the + * object's declarations beside the object (`objectActions`), as the console's + * two hosts do. + * + * The nodes that carry a `patch` are written through `undeclaredNode`: the + * key is the subject here, and the typed node face refuses it once the spec + * retires it. + */ + +import { describe, it, expect, vi, beforeEach, afterEach, type Mock } from 'vitest'; +import { render, screen, fireEvent, waitFor, within, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; +import { createServerActionHandler, globalUndoManager } from '@object-ui/core'; +import type { ActionContext, ActionDef, ActionResult } from '@object-ui/core'; +import { undeclaredNode } from '@object-ui/test-support'; +import { ActionProvider, RecordContextProvider, SchemaRenderer } from '@object-ui/react'; +// Module-scope side-effect imports, so the renderers are registered before the +// first render: the light `dom` project does not load the components graph. +// Module scope, not a `beforeAll`, per AGENTS.md 测试纪律. +import '../action-button'; +import '../action-icon'; +import '../action-group'; +import '../action-menu'; + +/** The record the action runs on, as the record page carries it. */ +const RECORD = { id: 't1', status: 'open', priority: 'low', note: 'was' }; + +/** The object's declared actions, as the host publishes them. */ +const DECLARATIONS = [ + { name: 'close_task', label: 'Close', operation: 'update', patch: { status: 'done' }, undoable: true }, + { + name: 'close_with_note', + label: 'Close with note', + operation: 'update', + patch: { status: 'done' }, + params: [{ name: 'note', type: 'text', label: 'Note' }], + undoable: true, + }, +]; + +let toast: Mock<(message: string, options?: { type?: string; undo?: { label?: string } }) => void>; +let fetchSpy: Mock<(url: string, init?: RequestInit) => Promise>; +/** Every def the `script` dispatch was handed, before it POSTs. */ +let dispatched: ActionDef[]; +let script: (action: ActionDef, ctx?: ActionContext) => Promise; + +beforeEach(() => { + toast = vi.fn(); + fetchSpy = vi.fn(async () => + new Response(JSON.stringify({ success: true, data: {} }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }), + ); + dispatched = []; + const route = createServerActionHandler({ fetch: fetchSpy as never, resolveObject: () => 'task' }); + script = async (action, ctx) => { + dispatched.push(action); + return route(action, ctx); + }; + globalUndoManager.clear(); +}); + +afterEach(() => { + cleanup(); + globalUndoManager.clear(); +}); + +/** + * Mount `node` on a record page, inside a host runner. `declarations` is what + * the host publishes as `objectActions`; `null` publishes none. + */ +function onRecordPage( + node: Record, + declarations: readonly unknown[] | null = DECLARATIONS, +) { + return render( + ({ note: 'typed by the user' })) as never} + > + + + + , + ); +} + +/** Wait for the runner to report back through the toast. */ +const settled = () => waitFor(() => expect(toast).toHaveBeenCalled()); + +/** The success toast's options; `undo` is set exactly when the toast offers Undo. */ +const successToast = () => toast.mock.calls.find(([, options]) => options?.type === 'success')?.[1]; + +/** The request the route was sent. */ +const sentBody = () => JSON.parse(String(fetchSpy.mock.calls.at(-1)?.[1]?.body)); + +/** A row that names `close_task` and carries a `patch` of its own. */ +const ROW = { + name: 'close_task', + label: 'Close', + operation: 'update', + patch: { priority: 'high' }, + undoable: true, +}; + +/** Radix opens a dropdown on `pointerdown` and mounts its content in a portal. */ +async function openMenu(trigger: HTMLElement) { + fireEvent.pointerDown(trigger, { button: 0, ctrlKey: false, pointerType: 'mouse' }); + return screen.findByRole('menu'); +} + +/** Each surface, mounted with {@link ROW} and clicked the way a user clicks it. */ +const SURFACES: Array<[string, () => Promise]> = [ + [ + 'action:button', + async () => { + onRecordPage({ type: 'action:button', properties: ROW }); + fireEvent.click(screen.getByRole('button', { name: 'Close' })); + }, + ], + [ + 'action:icon', + async () => { + onRecordPage({ type: 'action:icon', properties: { ...ROW, icon: 'check' } }); + fireEvent.click(screen.getByRole('button')); + }, + ], + [ + 'action:group', + async () => { + onRecordPage({ type: 'action:group', actions: [ROW] }); + fireEvent.click(screen.getByRole('button', { name: 'Close' })); + }, + ], + [ + 'action:menu', + async () => { + onRecordPage({ type: 'action:menu', actions: [ROW] }); + const menu = await openMenu(screen.getByRole('button', { name: 'More actions' })); + fireEvent.click(within(menu).getByRole('menuitem', { name: 'Close' })); + }, + ], +]; + +describe('no action renderer forwards a `patch` (objectui#13022)', () => { + it.each(SURFACES)('%s hands the runner `operation` and no `patch`', async (_surface, click) => { + await click(); + await waitFor(() => expect(dispatched).toHaveLength(1)); + const def = dispatched[0] as unknown as Record; + // The renderer composed the def: it forwarded the row's `name` and + // `operation`, so the update path ran. It forwarded no `patch`. + expect(def).toMatchObject({ name: 'close_task', operation: 'update' }); + expect(Object.prototype.hasOwnProperty.call(def, 'patch')).toBe(false); + }); + + it('the request a row sends carries no field value, with or without a `patch` on the row', async () => { + // The request has carried the collected params alone since objectui#12941, + // so not forwarding the row's `patch` moves no request byte. + onRecordPage({ type: 'action:button', properties: ROW }); + fireEvent.click(screen.getByRole('button', { name: 'Close' })); + await settled(); + expect(sentBody()).toEqual({ recordId: 't1', params: {} }); + cleanup(); + toast.mockClear(); + const { patch: _rowPatch, ...withoutPatch } = ROW; + onRecordPage({ type: 'action:button', properties: withoutPatch }); + fireEvent.click(screen.getByRole('button', { name: 'Close' })); + await settled(); + expect(sentBody()).toEqual({ recordId: 't1', params: {} }); + }); +}); + +describe('Undo on a row covers the `patch` of the declaration its `name` names (objectui#13022)', () => { + it('a row with no `patch` of its own: Undo restores the field the declaration writes', async () => { + const { patch: _rowPatch, ...row } = ROW; + onRecordPage({ type: 'action:button', properties: row }); + fireEvent.click(screen.getByRole('button', { name: 'Close' })); + await settled(); + expect(successToast()?.undo).toBeDefined(); + expect(globalUndoManager.peekUndo()).toMatchObject({ + type: 'update', + objectName: 'task', + recordId: 't1', + undoData: { status: 'open' }, + redoData: { status: 'done' }, + }); + }); + + it('the `patch` of the declaration plus the collected params, which win over it', async () => { + onRecordPage({ + type: 'action:button', + properties: { + name: 'close_with_note', + label: 'Close with note', + operation: 'update', + undoable: true, + params: [{ name: 'note', type: 'text', label: 'Note' }], + }, + }); + fireEvent.click(screen.getByRole('button', { name: 'Close with note' })); + await settled(); + expect(globalUndoManager.peekUndo()).toMatchObject({ + undoData: { status: 'open', note: 'was' }, + redoData: { status: 'done', note: 'typed by the user' }, + }); + // The request carries the collected value alone; `status` is the route's. + expect(sentBody()).toEqual({ recordId: 't1', params: { note: 'typed by the user' } }); + }); + + it('the `patch` a row carries itself is never covered, the one its declaration carries is', async () => { + // The row says `priority`; the declaration it names writes `status`. The + // route writes `status`, so Undo restores `status` and leaves `priority`. + onRecordPage({ type: 'action:button', properties: ROW }); + fireEvent.click(screen.getByRole('button', { name: 'Close' })); + await settled(); + const undo = globalUndoManager.peekUndo(); + expect(undo?.undoData).toEqual({ status: 'open' }); + expect(undo?.redoData).toEqual({ status: 'done' }); + }); + + it('control: a host that publishes no declarations gives a patch-only row no Undo', async () => { + // Nothing names what the route wrote, so there is nothing to snapshot; the + // lit rows above are lit by the published declaration, not by the harness. + const { patch: _rowPatch, ...row } = ROW; + onRecordPage({ type: 'action:button', properties: row }, null); + fireEvent.click(screen.getByRole('button', { name: 'Close' })); + await settled(); + expect(successToast()).toBeDefined(); + expect(successToast()?.undo).toBeUndefined(); + expect(globalUndoManager.undoCount).toBe(0); + }); +}); diff --git a/packages/components/src/renderers/action/action-button.tsx b/packages/components/src/renderers/action/action-button.tsx index 354e1a84dc..70dbbd6e83 100644 --- a/packages/components/src/renderers/action/action-button.tsx +++ b/packages/components/src/renderers/action/action-button.tsx @@ -309,18 +309,19 @@ const ActionButtonRenderer = forwardRef< // (objectstack#6938). Sibling of `bodyExtra` above, same whitelist, // same failure mode. bodyShape: schema.bodyShape, - // The declarative single-record field write, forwarded as a PAIR. - // `operation` is authored BESIDE `type`, not inside it — the spec - // materializes `type: 'script'` here and `ActionType` gained no member — - // so a forward that carries `type` alone tells the runner nothing about - // this action, and `patch` (which holds the field values) would be - // dropped one hop before the runner. The result is the route POSTed an - // empty write and a green toast for having written nothing: the - // objectstack#6837 / objectui#2960 shape, on the key whose whole payload - // is the thing dropped. `@object-ui/core`'s runner dispatches on - // `operation` ahead of `type`; both keys must reach it. + // The declarative single-record field write. `operation` is authored + // BESIDE `type`, not inside it — the spec materializes `type: 'script'` + // here and `ActionType` gained no member — so a forward that carries + // `type` alone tells the runner nothing about this action. + // `@object-ui/core`'s runner dispatches on `operation` ahead of `type`. + // + // ⛔ No `patch` (objectui#13022). The route writes the `patch` of the + // DECLARATION this node's `name` names, never a row's own: + // `@objectstack/spec` 18 retires the row key. The runner reads that + // declaration by name from what its host publishes, for the Undo + // snapshot only; the request has carried no `patch` since + // objectui#12941. operation: schema.operation, - patch: schema.patch, confirmText: schema.confirmText, successMessage: schema.successMessage, // Success copy per handler outcome (spec 17.6.0, objectui#11344): the diff --git a/packages/components/src/renderers/action/action-group.tsx b/packages/components/src/renderers/action/action-group.tsx index d9a6e247f4..c9d3a8496e 100644 --- a/packages/components/src/renderers/action/action-group.tsx +++ b/packages/components/src/renderers/action/action-group.tsx @@ -363,12 +363,12 @@ const ActionGroupRenderer = forwardRef; actions: unknown[] } + +// The same context `ActionProvider` takes. `objectFields` and `objectActions` +// are read for `objectName`'s object only. +new ActionRunner({ + objectName: 'task', + objectFields: taskDef.fields, + objectActions: taskDef.actions, +}) +``` + +- The list is read only for the context's own object. A dispatch to another + object, or a host that publishes no list, reads the dispatched action's own + `patch`: a host that dispatches an object's action whole hands it in. +- A page action row's own `patch` is never covered, and no action renderer + forwards one: `@objectstack/spec` 18 retires the key on the row. A row that + names an action whose declaration has no `patch` snapshots the collected + params alone. + ### Display time zone (`setDisplayTimeZone`) The date and datetime faces this package exports (`formatDate`, diff --git a/packages/core/src/actions/ActionRunner.ts b/packages/core/src/actions/ActionRunner.ts index edf7176c00..79aef21170 100644 --- a/packages/core/src/actions/ActionRunner.ts +++ b/packages/core/src/actions/ActionRunner.ts @@ -441,6 +441,12 @@ export interface ActionDef { * The ROUTE applies it, from the declaration it resolves by name; this * runner never sends it (objectui#12941) and reads it only to know which * fields an Undo must restore. + * + * It is a DECLARATION's key here: a host that dispatches an object's action + * whole hands it along. No action renderer forwards a page row's own `patch` + * (objectui#13022, following `@objectstack/spec` 18's retirement of the row + * key), and the runner reads the host-published declaration ahead of this + * one (`declaredUpdatePatch`). */ patch?: SpecActionInput['patch']; /** Execution mode of the action. */ @@ -937,6 +943,73 @@ function readContextObjectFields(context: ActionContext, objectName: string): Fi return fields && typeof fields === 'object' ? (fields as FieldContainerLike) : undefined; } +/** + * The declared actions the host published for `objectName`, or `undefined` + * (objectui#13022). + * + * A host publishes `objectActions` (the object's `actions` list, the + * declarations the platform action route resolves an action's `name` against) + * beside `objectName` and `objectFields`; `useConsoleActionRuntime` and + * `RecordDetailView` both do. Answered only for THAT object, for the reason + * {@link readContextObjectFields} gives: a dispatch that retargets another + * object names a declaration this list does not hold. + */ +function readContextObjectActions( + context: ActionContext, + objectName: string | undefined, +): readonly unknown[] | undefined { + if (!objectName || readContextObjectName(context) !== objectName) return undefined; + const actions: unknown = context.objectActions; + return Array.isArray(actions) ? actions : undefined; +} + +/** A `patch` as the route reads one: a plain object's own members, else none. */ +function patchMembers(patch: unknown): Record { + return patch && typeof patch === 'object' && !Array.isArray(patch) + ? { ...(patch as Record) } + : {}; +} + +/** + * The static values the platform action route writes for an + * `operation: 'update'` dispatch: the `patch` of the DECLARATION the action's + * `name` names (objectui#13022). + * + * The route resolves that declaration by the dispatch's object and the + * action's `name`, and writes its `patch` under the params it is sent + * (`declarativeUpdateWrite`). A page action row's own `patch` is never that + * write: `@objectstack/spec` 18 retires the key on `action:button`, + * `action:icon` and the `action:group` / `action:menu` member, and none of + * those renderers forwards it. So the declaration is read from one of two + * places, and no third: + * + * 1. The declarations the host published for the dispatch's object + * ({@link readContextObjectActions}), when they name this action: the + * lookup the route performs. A declaration with no `patch` writes the + * collected params alone. + * 2. Otherwise the def itself. A host that dispatches an object's action whole + * (`ObjectGrid`'s rows, `DeclaredActionsBar`, `RelatedRecordActionsBridge`, + * `page:header`) hands the runner the declaration, `patch` included. A page + * row reaches the runner through a renderer, which forwards no `patch`, so + * this reads nothing for one. + * + * Read for the Undo snapshot only; ⛔ never sent (objectui#12941). + */ +function declaredUpdatePatch( + action: ActionDef, + context: ActionContext, + objectName: string | undefined, +): Record { + const name = typeof action.name === 'string' && action.name !== '' ? action.name : undefined; + const declared = name === undefined + ? undefined + : readContextObjectActions(context, objectName)?.find( + (candidate) => + !!candidate && typeof candidate === 'object' && (candidate as { name?: unknown }).name === name, + ); + return patchMembers(declared ? (declared as { patch?: unknown }).patch : action.patch); +} + /** * Whether opening FormView `viewName` from a record of `contextObject` would * cross an object boundary (objectui#4292). @@ -1797,6 +1870,14 @@ export class ActionRunner { * WINS — the author declared a default, and the user answered it. This * runner still computes that same merge, but only to know what the route * WROTE, which is what an Undo snapshot must cover; it is never sent. + * + * ## The `patch` in that merge is the DECLARATION's (objectui#13022) + * + * The route writes the `patch` of the declaration the action's `name` names, + * so that is the `patch` the snapshot covers: read by + * {@link declaredUpdatePatch} from the declarations the host published, else + * off a def that is the declaration itself. A page action row's own `patch` + * is never written (`@objectstack/spec` 18 retires it) and never covered. */ private async executeUpdateOperation(action: ActionDef): Promise { const dispatch = this.handlers.get('script'); @@ -1816,13 +1897,13 @@ export class ActionRunner { const collected: Record = (action.params && !Array.isArray(action.params)) ? { ...(action.params as Record) } : {}; - const patch: Record = - (action.patch && typeof action.patch === 'object' && !Array.isArray(action.patch)) - ? { ...(action.patch as Record) } - : {}; - // What the route WRITES: `patch` UNDER the collected params, the route's - // own merge (see the doc above). Read for the Undo snapshot only — ⛔ never + // The object the write lands on: the action's own, else the host's. + const objectName = action.objectName || readContextObjectName(this.context); + // What the route WRITES: the DECLARATION's `patch` UNDER the collected + // params, the route's own merge (see the doc above), never a page row's + // own `patch` (objectui#13022). Read for the Undo snapshot only — ⛔ never // the request: the route applies `patch` from its declaration. + const patch = declaredUpdatePatch(action, this.context, objectName); const written = { ...patch, ...collected }; // The row stash and the record selector are transport mechanics, not @@ -1854,7 +1935,6 @@ export class ActionRunner { // carries expanded is captured as its stored id, read against the field // map the host published for this object (objectui#11122). if (action.undoable && rowRecord && writtenFields.length > 0) { - const objectName = action.objectName || readContextObjectName(this.context); const recordId = collected.recordId ?? rowRecord.id; if (objectName && recordId != null) { const undoData = captureUpdateUndoData( diff --git a/packages/core/src/actions/__tests__/ActionRunner.declaredPatch-13022.test.ts b/packages/core/src/actions/__tests__/ActionRunner.declaredPatch-13022.test.ts new file mode 100644 index 0000000000..fe2f09410b --- /dev/null +++ b/packages/core/src/actions/__tests__/ActionRunner.declaredPatch-13022.test.ts @@ -0,0 +1,180 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * objectui#13022 — the Undo snapshot of an `operation: 'update'` action covers + * the `patch` of the DECLARATION the action's `name` names, plus the collected + * params. + * + * The platform action route resolves the declaration by the dispatch's object + * and the action's `name`, and writes `{ ...declaration.patch, ...params }` + * (`declarativeUpdateWrite`). So that is the write an Undo must restore. A page + * action row's own `patch` is never it: `@objectstack/spec` 18 retires the row + * key, and no action renderer forwards it. The runner reads the declaration + * from the `objectActions` its host publishes beside `objectName` (as the + * console's `useConsoleActionRuntime` and `RecordDetailView` do), and, when the + * host published none for the dispatch's object, off the def itself: a host + * that dispatches an object's action whole hands the declaration in. + * + * The dispatch is the real `createServerActionHandler` over a fetch stub, so + * every case also shows the request carries the collected params alone. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ActionRunner, type ActionDef } from '../ActionRunner'; +import { createServerActionHandler, type ServerActionFetch } from '../serverActionHandler'; + +/** A fetch stub answering the route's success envelope. */ +function okFetch() { + return vi.fn(async () => ({ ok: true, status: 200, json: async () => ({ success: true, data: {} }) })); +} + +/** The parsed JSON body of the last request a fetch stub was sent. */ +function sentBody(fetch: ReturnType): { recordId?: unknown; params?: unknown } { + const calls = fetch.mock.calls as unknown as Array<[string, { body: string }]>; + return JSON.parse(calls[calls.length - 1][1].body); +} + +/** The object's declared actions, as a host publishes them. */ +const DECLARATIONS = [ + { name: 'close_task', objectName: 'task', operation: 'update', patch: { status: 'done' }, undoable: true }, + { + name: 'set_note', + objectName: 'task', + operation: 'update', + params: [{ name: 'note', type: 'text' }], + undoable: true, + }, +]; + +/** The row the action runs on, stashed the way every invoking surface stashes it. */ +const ROW = { id: 't1', status: 'open', priority: 'low', note: 'was' }; + +/** A runner wired as a console host wires one, publishing `context`. */ +function wire(context: Record) { + const fetch = okFetch(); + const runner = new ActionRunner(context); + runner.registerHandler( + 'script', + createServerActionHandler({ fetch: fetch as unknown as ServerActionFetch, resolveObject: () => 'task' }), + ); + return { runner, fetch }; +} + +/** A row's dispatch: the row names its action and carries the record stash. */ +function rowDispatch(extra: Partial & { params?: Record } = {}): ActionDef { + const { params, ...rest } = extra; + return { + name: 'close_task', + operation: 'update', + undoable: true, + ...rest, + params: { ...(params ?? {}), _rowRecord: ROW }, + } as ActionDef; +} + +const PUBLISHED = { objectName: 'task', objectActions: DECLARATIONS }; + +describe("operation: 'update' — the snapshot covers the named declaration's `patch` (objectui#13022)", () => { + it('a row that carries no `patch`: Undo covers the field the declaration writes', async () => { + const { runner, fetch } = wire(PUBLISHED); + + const result = await runner.execute(rowDispatch()); + + expect(result.success).toBe(true); + expect(result.undo).toMatchObject({ + objectName: 'task', + recordId: 't1', + undoData: { status: 'open' }, + redoData: { status: 'done' }, + }); + // The route writes `status` from the declaration; the request carries none. + expect(sentBody(fetch)).toEqual({ recordId: 't1', params: {} }); + }); + + it('the declaration wins over a `patch` the def carries itself', async () => { + // A def built from a row that still carries its own `patch` (the spec + // accepts the key before 18): the route never writes it, so the snapshot + // covers `status`, the declaration's field, and not `priority`. + const { runner } = wire(PUBLISHED); + + const result = await runner.execute(rowDispatch({ patch: { priority: 'high' } as never })); + + expect(result.undo?.undoData).toEqual({ status: 'open' }); + expect(result.undo?.redoData).toEqual({ status: 'done' }); + }); + + it('a declaration with no `patch`: the snapshot is the collected params alone', async () => { + const { runner, fetch } = wire(PUBLISHED); + runner.setParamCollectionHandler(async () => ({ note: 'typed' })); + + const result = await runner.execute(rowDispatch({ + name: 'set_note', + actionParams: [{ name: 'note', type: 'text' }] as never, + })); + + expect(result.undo?.undoData).toEqual({ note: 'was' }); + expect(result.undo?.redoData).toEqual({ note: 'typed' }); + expect(sentBody(fetch)).toEqual({ recordId: 't1', params: { note: 'typed' } }); + }); + + it('a field the declaration writes that the row does not carry: no Undo, never an invented prior value', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const { runner } = wire(PUBLISHED); + const { status: _status, ...rowWithoutStatus } = ROW; + + const result = await runner.execute({ + name: 'close_task', + operation: 'update', + undoable: true, + params: { _rowRecord: rowWithoutStatus }, + } as ActionDef); + + expect(result.success).toBe(true); + expect(result.undo).toBeUndefined(); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining('offers no Undo'), + expect.objectContaining({ action: 'close_task', missing: ['status'] }), + ); + } finally { + warn.mockRestore(); + } + }); +}); + +describe("operation: 'update' — when the host published no declaration for the dispatch's object", () => { + it('a def that IS the declaration (a spread-based host) is read whole, `patch` included', async () => { + const { runner } = wire({ objectName: 'task' }); + + const result = await runner.execute(rowDispatch({ patch: { status: 'done' } as never })); + + expect(result.undo?.undoData).toEqual({ status: 'open' }); + expect(result.undo?.redoData).toEqual({ status: 'done' }); + }); + + it('a dispatch to ANOTHER object does not read this object\'s declarations', async () => { + // A related-list row on the record page: the child object's action, handed + // in whole with its own `patch`. The host's list holds the PARENT's + // declarations, so a same-named one there is not this action's. + const { runner } = wire({ + objectName: 'project', + objectActions: [{ name: 'close_task', operation: 'update', patch: { phase: 'closed' } }], + }); + + const result = await runner.execute(rowDispatch({ objectName: 'task', patch: { status: 'done' } as never })); + + expect(result.undo).toMatchObject({ objectName: 'task', undoData: { status: 'open' }, redoData: { status: 'done' } }); + }); + + it('control: a row with no `patch` and no published declaration offers no Undo', async () => { + // Nothing names a written field, so there is nothing to restore; the lit + // rows above are lit by the declaration, not by the harness. + const { runner } = wire({ objectName: 'task' }); + + const result = await runner.execute(rowDispatch()); + + expect(result.success).toBe(true); + expect(result.undo).toBeUndefined(); + }); +}); diff --git a/packages/core/src/utils/predicate-fields.ts b/packages/core/src/utils/predicate-fields.ts index ea10b254a0..dc4acab539 100644 --- a/packages/core/src/utils/predicate-fields.ts +++ b/packages/core/src/utils/predicate-fields.ts @@ -133,7 +133,11 @@ function defaultFromRowKey(param: unknown): unknown { * Each source is the one a writer actually merges into the written set: * * - `patch`: the static field values of `operation: 'update'`, merged under the - * collected params (`ActionRunner`'s `executeUpdateOperation`); + * collected params (`ActionRunner`'s `executeUpdateOperation`). Every def + * this harvest reads is a definition its writer applies whole (a view's + * resolved row def, a `bulkActionDefs` entry, an object's action), never a + * page action row, so this is the declared `patch` the write applies: the + * one the runner's Undo snapshot reads too (objectui#13022); * - each param's COLLECTED key, `name ?? field`: the key the param dialog * stores its answer under (`paramName` in `app-shell`'s * `resolveActionParams`). This is the opposite precedence to diff --git a/scripts/check-action-forward-parity.mjs b/scripts/check-action-forward-parity.mjs index cb4f078d65..e586594f86 100644 --- a/scripts/check-action-forward-parity.mjs +++ b/scripts/check-action-forward-parity.mjs @@ -305,6 +305,25 @@ export const JUSTIFIED = { ]) ), + // ── Read off the declaration, never off the node ─────────────────────────── + ...Object.fromEntries( + ["action:button", "action:icon", "action:group", "action:menu"].map((surface) => [ + `${surface}:patch`, + { + reason: + "Correctly omitted (objectui#13022). The platform action route writes the `patch` of the " + + "DECLARATION the action's `name` names, never a node's own, and @objectstack/spec 18 " + + "retires a page action row's `patch` on these four surfaces. The runner reads that " + + "declaration by name from the `objectActions` its host publishes " + + "(`declaredUpdatePatch` in ActionRunner.ts), which also covers a declaration `action:bar` " + + "draws through these renderers; it reads `action.patch` only off a def that IS the " + + "declaration, which the spread-based hosts dispatch whole. The request carries no " + + "`patch` since objectui#12941, so the omission moves no request byte.", + issue: 13022, + }, + ]) + ), + // ── Read only to improve a diagnostic ────────────────────────────────────── ...Object.fromEntries( ["action:button", "action:icon", "action:group", "action:menu"].map((surface) => [