diff --git a/.changeset/action-onsuccess-forward-5493.md b/.changeset/action-onsuccess-forward-5493.md new file mode 100644 index 0000000000..b74a69cef5 --- /dev/null +++ b/.changeset/action-onsuccess-forward-5493.md @@ -0,0 +1,38 @@ +--- +'@object-ui/components': patch +--- + +`action:button`, `action:icon`, `action:group` and `action:menu` now forward the +declared `onSuccess` block, so the post-success navigation an author writes in metadata +actually happens on those four surfaces (objectui#5493). + +`onSuccess` — spec's closed strict `{ navigate, openIn }` object — became authorable on +`ActionSchema` with the `@objectstack/spec` 17.1.0 pin bump (objectui#5328), and the +runner has read it off the **forwarded** def since objectui#5221: +`ActionRunner.handlePostExecution` → `readOnSuccessNavigation` → `navigateOnSuccess`, +which hops through the app's own `navigationHandler` (a real SPA route change, immune to +popup blocking). Between those two halves sat these four forward whitelists, which never +carried the key. The action succeeded, the toast said so, and the declared hop silently +did not happen — the same "shipped green while dropped one hop before the runner" class +as `bodyExtra` (objectstack#6837), `bodyShape` (objectstack#6938) and `resultDialog` +(objectui#3646). + +Reachability before this change was a function of which host rendered the action: the +full-def-spread hosts (`DeclaredActionsBar`, `ObjectGrid.onActionDef`, +`RelatedRecordActionsBridge`, `useNavActionDispatch`) already carried it through, so the +same declaration hopped there and did nothing here — and on an `action:bar`, whether an +action lands inline (`action:button`) or in the overflow (`action:menu`) is decided by +`maxVisible`, 3 desktop / 1 mobile. The declared navigation therefore depended on +viewport width. + +Pinned end to end in +`packages/components/src/renderers/action/__tests__/action-onSuccess-forward.test.tsx`: +one row per surface drives the real renderer through the real runner and asserts the +`${result.*}`-interpolated url reaches `onNavigate`, with `openIn` exercised on both +branches. Each row asserts the action executed first, so a zero-navigation reading +cannot be a harness that did nothing. + +The four matching `KNOWN_GAPS` entries in `scripts/check-action-forward-parity.mjs` are +deleted — that ledger is ratcheted, so a stale entry excusing a key that is now +forwarded fails the gate. `element:button` is untouched and stays correct: `onSuccess` +is not on spec's `InlineActionSchema` pick list, so that surface never owed it. 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 46b8baf841..cb6d5c46f7 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 @@ -85,12 +85,17 @@ const CONTEXT = { target: 'contextWins', extraFromHost: 'present' }; /** * The exact key order `action:button` composes, top to bottom of its literal. * `params` sits where `...paramsPayload` sits — ninth, not last. + * + * `onSuccess` is the tail entry as of objectui#5493 (the declared post-success + * hop). A whitelist key APPENDED moves nothing, which is 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. */ const BUTTON_ORDER = [ 'type', 'name', 'label', 'description', 'target', 'openIn', 'endpoint', 'method', 'params', 'bodyExtra', 'bodyShape', 'confirmText', 'successMessage', 'errorMessage', 'refreshAfter', - 'undoable', 'recordIdField', 'locations', 'toast', 'resultDialog', + 'undoable', 'recordIdField', 'locations', 'toast', 'resultDialog', 'onSuccess', ]; /** `action:icon` has no `paramsPayload`; `params` is an ordinary explicit key. */ @@ -98,7 +103,7 @@ const ICON_ORDER = [ 'type', 'name', 'label', 'description', 'target', 'openIn', 'endpoint', 'method', 'params', 'bodyExtra', 'bodyShape', 'confirmText', 'successMessage', 'errorMessage', 'refreshAfter', - 'locations', 'toast', 'resultDialog', + 'locations', 'toast', 'resultDialog', 'onSuccess', ]; // See action-bodyExtra-forward.test.tsx for why this is not diff --git a/packages/components/src/renderers/action/__tests__/action-onSuccess-forward.test.tsx b/packages/components/src/renderers/action/__tests__/action-onSuccess-forward.test.tsx new file mode 100644 index 0000000000..154f0f9def --- /dev/null +++ b/packages/components/src/renderers/action/__tests__/action-onSuccess-forward.test.tsx @@ -0,0 +1,173 @@ +/** + * 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#5493 — `ActionSchema.onSuccess` must SURVIVE the hop on all four + * declared action surfaces. + * + * The key became authorable on `ActionSchema` with the `@objectstack/spec` + * 17.1.0 pin bump (objectui#5328) and the runner has read it off the forwarded + * def since objectui#5221 (`handlePostExecution` → `readOnSuccessNavigation` → + * `navigateOnSuccess` → the app's own `navigationHandler`). Between those two + * halves sat these four whitelists, which never carried the key: the action + * succeeded, the declared post-success hop silently did not happen. + * + * ## What these pins assert, and why it is not "the action succeeded" + * + * An assertion that the action ran passes in BOTH worlds — it cannot see a + * dropped key. Each row here asserts the two facts that differ: + * + * 1. the handler is handed a def whose `onSuccess` is the authored block + * (the forward itself), and + * 2. the runner performs the hop — `onNavigate` is called with the + * `${result.*}`-INTERPOLATED url and the declared `openIn` branch. + * + * (2) is the artefact that matters; a url only this declaration can produce + * (`/app/crm/contacts/rec_42`, interpolated from the handler's own payload) + * is what keeps it off a truthiness check. + * + * ## The positive control is in every row + * + * Each row asserts `api` was called exactly once BEFORE asserting on the + * navigation spy. A zero-navigation reading therefore cannot be "the harness + * never executed anything" (an unmounted renderer, an un-clicked button, an + * assertion that ran before the async execute settled) — the two failure + * modes are distinguishable from the failure message alone. + * + * ## Why one row per surface + * + * These are four separate whitelists, not one helper: a fix or a regression on + * one renderer must show up as exactly one red row. `openIn` is exercised on + * both branches across the rows (`'self'` on three, `'newTab'` on `action:menu`) + * so the pin covers the forwarded BLOCK rather than only its `navigate` string. + * + * `element:button` (spec's `InlineActionSchema` pick list) is deliberately + * absent: `onSuccess` is not on that pick list, so that surface never owed it — + * the same split `scripts/check-action-forward-parity.mjs` derives rather than + * registers. + */ + +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; +import { render, screen, waitFor, fireEvent } from '@testing-library/react'; +import React from 'react'; +import { ComponentRegistry } from '@object-ui/core'; +import type { ActionContext, ActionDef, ActionResult } from '@object-ui/core'; +import { ActionProvider } from '@object-ui/react'; +// Module-scope side-effect imports — these renderers register themselves with +// the ComponentRegistry, and the light `dom` project does not load the +// `@object-ui/components` graph. Module scope, not a `beforeAll`, per +// AGENTS.md §测试纪律. +import '../action-button'; +import '../action-icon'; +import '../action-group'; +import '../action-menu'; + +/** The handler's own return value — what `${result.*}` reads (objectui#2904). */ +const PAYLOAD = { id: 'rec_42' }; + +/** The authored block, spelled as the spec declares it: `{ navigate, openIn }`. */ +const ONSUCCESS_SELF = { navigate: '/app/crm/contacts/${result.id}', openIn: 'self' } as const; +const ONSUCCESS_NEWTAB = { navigate: '/app/crm/contacts/${result.id}', openIn: 'newTab' } as const; + +/** The customer shape (clone a record, then jump to the clone). */ +const clone = (onSuccess: unknown, extra: Record = {}) => ({ + name: 'clone_record', + label: 'Clone', + type: 'api', + target: '/api/v1/records/clone', + locations: ['list_toolbar'], + onSuccess, + ...extra, +}); + +// Typed with the signatures the provider's props declare, not +// `ReturnType` — see action-bodyExtra-forward.test.tsx +// (objectui#4040). +let api: Mock<(action: ActionDef, ctx: ActionContext) => Promise>; +let nav: Mock<(url: string, options?: { external?: boolean; newTab?: boolean }) => void>; + +beforeEach(() => { + api = vi.fn(async () => ({ success: true, data: PAYLOAD })); + nav = vi.fn(); +}); + +const renderSurface = (node: React.ReactNode) => + render( + + {node} + , + ); + +/** The renderer under test, straight off the registry (as `action:bar` gets it). */ +function surface(type: string, schema: Record) { + const C = ComponentRegistry.get(type); + if (!C) throw new Error(`${type} is not registered`); + return ; +} + +/** The def the `api` handler was handed — i.e. what reached the runner. */ +const executedDef = () => api.mock.calls[0][0] as ActionDef & { onSuccess?: unknown }; + +/** + * The two facts, in order: the action ran (positive control), the block was + * forwarded, and the hop happened with the interpolated url. + */ +async function expectHop(expected: { openIn: 'self' | 'newTab'; block: unknown }) { + await waitFor(() => expect(api).toHaveBeenCalledTimes(1)); + expect(executedDef().onSuccess).toEqual(expected.block); + + await waitFor(() => expect(nav).toHaveBeenCalledTimes(1)); + const [url, options] = nav.mock.calls[0]; + expect(url).toBe('/app/crm/contacts/rec_42'); + expect(options?.newTab).toBe(expected.openIn === 'newTab'); +} + +describe('ActionSchema.onSuccess reaches the runner from every declared surface (#5493)', () => { + it('action:button — a clicked button performs the declared post-success hop', async () => { + renderSurface(surface('action:button', clone(ONSUCCESS_SELF))); + + fireEvent.click(screen.getByRole('button', { name: 'Clone' })); + + await expectHop({ openIn: 'self', block: ONSUCCESS_SELF }); + }); + + it('action:icon — the icon-only surface hops too, not only the labelled one', async () => { + // Same declaration, dense layout. Which renderer an action gets is a host's + // choice (`component`), so a whitelist that carries the key on one surface + // and drops it on another makes the hop a function of the layout. + renderSurface(surface('action:icon', clone(ONSUCCESS_SELF))); + + fireEvent.click(screen.getByRole('button', { name: 'Clone' })); + + await expectHop({ openIn: 'self', block: ONSUCCESS_SELF }); + }); + + it('action:group — an inline group member hops', async () => { + renderSurface(surface('action:group', { type: 'action:group', actions: [clone(ONSUCCESS_SELF)] })); + + fireEvent.click(screen.getByRole('button', { name: 'Clone' })); + + await expectHop({ openIn: 'self', block: ONSUCCESS_SELF }); + }); + + it("action:menu — the overflow surface hops, and carries openIn: 'newTab'", async () => { + // `autoTrigger` runs `handleExecute`, the identical function a click on the + // menu item calls, without opening the Radix dropdown (whose + // pointerdown-driven portal is flaky to synthesize in happy-dom — see + // `action-group-dropdown-visible.test.tsx`). The `newTab` branch rides here + // so the pin covers the forwarded BLOCK, not just its `navigate` string. + renderSurface( + surface('action:menu', { + type: 'action:menu', + actions: [clone(ONSUCCESS_NEWTAB, { autoTrigger: true })], + }), + ); + + await expectHop({ openIn: 'newTab', block: ONSUCCESS_NEWTAB }); + }); +}); diff --git a/packages/components/src/renderers/action/action-button.tsx b/packages/components/src/renderers/action/action-button.tsx index 11d94a5b8b..454e277ff1 100644 --- a/packages/components/src/renderers/action/action-button.tsx +++ b/packages/components/src/renderers/action/action-button.tsx @@ -190,6 +190,17 @@ const ActionButtonRenderer = forwardRef< // backup codes). Without this forward the ActionRunner falls // back to the success toast and the user loses the value. resultDialog: (schema as any).resultDialog, + // Declared post-success navigation — spec's closed strict + // `{ navigate, openIn }` block, authorable on `ActionSchema` since + // @objectstack/spec 17.1.0 (objectui#5328). The runner reads it off + // the FORWARDED def at execute time (`handlePostExecution` → + // `readOnSuccessNavigation` → `navigateOnSuccess`, which hops through + // the app's own `navigationHandler`). Dropped here, the action + // succeeded and the declared hop silently never happened — + // objectui#5493, the same shape as `bodyShape` / `resultDialog` + // above. Cast because the key is spec-owned and not spelled on + // `@object-ui/types`' renderer view, exactly as `resultDialog` is. + onSuccess: (schema as any).onSuccess, }; await execute({ ...forwarded, ...localContext }); diff --git a/packages/components/src/renderers/action/action-group.tsx b/packages/components/src/renderers/action/action-group.tsx index 241e4a1b6d..e272a16655 100644 --- a/packages/components/src/renderers/action/action-group.tsx +++ b/packages/components/src/renderers/action/action-group.tsx @@ -273,6 +273,10 @@ const ActionGroupRenderer = forwardRef [ - `${surface}:onSuccess`, - { - reason: - "Newly OWED by the `@objectstack/spec` 17.1.0 pin bump (objectui#5328), not newly " + - "dropped: the runner has honoured it all along (ActionRunner.ts:1197 reads " + - "`action.onSuccess` and :1198 runs the chained defs), and 17.1.0 supplied the missing " + - "half by declaring the key authorable on `ActionSchema` — 0 occurrences in 17.0.0's " + - "`dist/**/*.d.ts`, 53 in 17.1.0. Both halves of the owed-set therefore hold for the " + - "first time on the bump. Forwarding it is capability WIRING, the same class the pin " + - "bump defers to its dependants (#5074 `viewMode`, #5042 `ListMapConfigSchema`), so it " + - "is filed rather than ridden in: objectui#5493. `element:button` is absent from this " + - "list because `onSuccess` is not on spec's `InlineActionSchema` pick list, so that " + - "surface never owed it.", - issue: 5493, - }, - ]) - ), }; // ── Opaque spreads ───────────────────────────────────────────────────────────