fix(components): forward the declared onSuccess block on all four action surfaces

.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.

packages/components/src/renderers/action/__tests__/action-forward-precedence.test.tsx

@@ -85,20 +85,25 @@ 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. */
 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

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<string, unknown> = {}) => ({
+ 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<typeof vi.fn>` — see action-bodyExtra-forward.test.tsx
+// (objectui#4040).
+let api: Mock<(action: ActionDef, ctx: ActionContext) => Promise<ActionResult>>;
+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(
+ <ActionProvider handlers={{ api }} onNavigate={nav} onToast={vi.fn()}>
+ {node}
+ </ActionProvider>,
+ );
+
+/** The renderer under test, straight off the registry (as `action:bar` gets it). */
+function surface(type: string, schema: Record<string, unknown>) {
+ const C = ComponentRegistry.get(type);
+ if (!C) throw new Error(`${type} is not registered`);
+ return <C schema={schema as never} />;
+}
+
+/** 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 });
+ });
+});

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 });

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

@@ -273,6 +273,10 @@ const ActionGroupRenderer = forwardRef<HTMLDivElement, { schema: ActionGroupSche
  // OAuth secret). Without it the runner falls back to the success
  // toast and the value the user was meant to copy is gone.
  resultDialog: (action as any).resultDialog,
+ // See action-button.tsx — the declared post-success hop
+ // (objectui#5493). The runner reads it off the forwarded def; dropped
+ // here the action succeeds and the authored navigation never runs.
+ onSuccess: (action as any).onSuccess,
  });
  },
  [execute],

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

@@ -123,6 +123,10 @@ const ActionIconRenderer = forwardRef<
  // OAuth secret). Without it the runner falls back to the success
  // toast and the value the user was meant to copy is gone.
  resultDialog: (schema as any).resultDialog,
+ // See action-button.tsx — the declared post-success hop
+ // (objectui#5493). The runner reads it off the forwarded def; dropped
+ // here the action succeeds and the authored navigation never runs.
+ onSuccess: (schema as any).onSuccess,
  };
 
  await execute({ ...forwarded, ...localContext });

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

@@ -252,6 +252,11 @@ const ActionMenuRenderer = forwardRef<HTMLButtonElement, { schema: ActionMenuSch
  // See action-button.tsx — overflow-menu actions need the
  // resultDialog spec forwarded too or the one-shot reveal is lost.
  resultDialog: (action as any).resultDialog,
+ // See action-button.tsx — the declared post-success hop
+ // (objectui#5493). An overflow action must hop like its inline
+ // twin, or the `action:bar` `maxVisible` split decides whether the
+ // declared navigation runs.
+ onSuccess: (action as any).onSuccess,
  });
  } finally {
  setLoading(false);

scripts/check-action-forward-parity.mjs

@@ -315,25 +315,6 @@ export const KNOWN_GAPS = {
  },
  ])
  ),
- ...Object.fromEntries(
- ["action:button", "action:icon", "action:group", "action:menu"].map((surface) => [
- `${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 ───────────────────────────────────────────────────────────