fix(grid): a legacy string row action runs instead of green-toasting a no-op (#2960)

.changeset/legacy-row-actions-dead-end.md

@@ -0,0 +1,37 @@
+---
+"@object-ui/plugin-grid": patch
+"@object-ui/core": patch
+---
+
+fix(grid): a legacy string row action runs instead of green-toasting a no-op — objectui#2960
+
+A list view declaring `rowActions: ['convert_lead']` rendered a menu item that
+performed **zero network requests** and reported success. Where the object also
+declared the same action with `locations: ['list_item']`, the row menu showed a
+working entry and a dead duplicate of it side by side.
+
+**The name never became an action.** `ObjectGrid` dispatched the legacy form as
+`{ type: <action name>, params: { record } }` — the action *name* landing in the
+runner's `type` slot, never resolved against the object's action defs. It
+matches no built-in type and (absent a handler registered under that exact name)
+no handler either, so it fell through to `ActionRunner.executeActionSchema`,
+which returned `{success: true, reload: true, close: true}` for a schema with
+nothing in it. `handlePostExecution` then fired the green "Action completed
+successfully" toast.
+
+Two changes, either of which would have surfaced the bug:
+
+**① `ObjectGrid` resolves legacy names against `objectDef.actions`.** A name
+that matches a declared action is promoted to that def and dispatched through
+the same path as a `list_item` action — so it actually runs, and it picks up the
+def's label, `visible`/`disabled` predicates, param dialog and capability gate,
+none of which the string form could carry. A name that matches an action already
+rendered as a def is dropped, which is what removes the dead twin. Names that
+resolve to nothing are still dispatched by name, since a consumer may have
+registered a runner handler under exactly that name.
+
+**② `ActionRunner`'s empty-schema fallthrough fails loudly.** It no longer
+reports success for an action it never ran: a dispatch with no registered
+handler and no `api`/`endpoint`/`navigate`/`redirect`/`onClick` returns a
+failure naming the action. Schema-only shapes that *do* declare something — a
+bare `redirect`, an explicit `reload`/`close` — run exactly as before.

packages/core/src/actions/ActionRunner.ts

@@ -884,10 +884,40 @@ export class ActionRunner {
     };
   }
 
+  /**
+   * Terminal fallback for a dispatch the switch above did not recognize: no
+   * known `type`, no registered handler, no `navigate` / `api` / `endpoint` /
+   * `onClick`. Some schema-only shapes are still runnable here — an action
+   * whose whole payload is a `redirect`, or an explicit `reload` / `close`
+   * directive — so this stays a real execution path.
+   *
+   * What it must never do is report success for an action it did not run. It
+   * used to return `{ success: true, reload: true, close: true }` for a
+   * COMPLETELY EMPTY schema, which is how a legacy string row action —
+   * dispatched as `{ type: '<action name>' }`, the name landing in the `type`
+   * slot — reached the user as a green "Action completed successfully" toast
+   * having issued zero requests (objectui#2960). An unresolvable dispatch is a
+   * wiring bug; surface it instead of laundering it into a success.
+   */
   private async executeActionSchema(action: ActionDef): Promise<ActionResult> {
+    const hasApi = !!(action.api || action.endpoint);
+    // `reload`/`close` default to true downstream, so only an EXPLICIT `true`
+    // counts as declared intent — otherwise every empty schema would look like
+    // it asked for a reload and slip back through this guard.
+    const declaresBehavior =
+      hasApi || !!action.onClick || !!action.redirect
+      || action.reload === true || action.close === true;
+    if (!declaresBehavior) {
+      const label = action.name || action.type || action.actionType || 'action';
+      return {
+        success: false,
+        error: `Action "${label}" is not executable: no handler is registered under that name and it declares no api, endpoint, navigate, redirect or onClick.`,
+      };
+    }
+
     const result: ActionResult = { success: true };
 
-    if (action.api || action.endpoint) {
+    if (hasApi) {
       const apiResult = await this.executeAPI(action);
       if (!apiResult.success) return apiResult;
       result.data = apiResult.data;

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

@@ -55,6 +55,57 @@ describe('ActionRunner', () => {
     });
   });
 
+  // ==========================================================================
+  // Unresolvable dispatch (objectui#2960)
+  // ==========================================================================
+
+  describe('unresolvable dispatch', () => {
+    it('fails loudly when the schema declares nothing executable', async () => {
+      // A legacy string row action reaches the runner as `{ type: <name> }` —
+      // the NAME in the `type` slot. It matches no built-in type and no
+      // registered handler, so there is nothing to run. This used to return
+      // `{ success: true, reload: true, close: true }` after issuing zero
+      // requests.
+      const result = await runner.execute({ type: 'convert_lead', params: { record: { id: 1 } } });
+
+      expect(result.success).toBe(false);
+      expect(result.error).toContain('convert_lead');
+      expect(result.reload).toBeUndefined();
+    });
+
+    it('does not toast success for an action it never ran', async () => {
+      const toastHandler = vi.fn();
+      runner.setToastHandler(toastHandler);
+
+      await runner.execute({ type: 'convert_lead' });
+
+      expect(toastHandler).toHaveBeenCalledTimes(1);
+      expect(toastHandler).toHaveBeenCalledWith(
+        expect.stringContaining('convert_lead'),
+        expect.objectContaining({ type: 'error' }),
+      );
+    });
+
+    it('still runs a handler registered under the action name', async () => {
+      // The by-name dispatch path is how consumers wire custom row actions;
+      // the guard must not close it.
+      const handler = vi.fn().mockResolvedValue({ success: true });
+      runner.registerHandler('convert_lead', handler);
+
+      const result = await runner.execute({ type: 'convert_lead' });
+
+      expect(result.success).toBe(true);
+      expect(handler).toHaveBeenCalledOnce();
+    });
+
+    it('still runs a typeless action that only declares a redirect', async () => {
+      const result = await runner.execute({ redirect: '/leads/1' });
+
+      expect(result.success).toBe(true);
+      expect(result.redirect).toBe('/leads/1');
+    });
+  });
+
   // ==========================================================================
   // Conditions and disabled
   // ==========================================================================

packages/plugin-grid/src/ObjectGrid.tsx

@@ -44,6 +44,7 @@ import { useGroupedData } from './useGroupedData';
 import { GroupRow } from './GroupRow';
 import { useColumnSummary } from './useColumnSummary';
 import { resolveRowCrudAffordances } from './rowCrudAffordances';
+import { resolveLegacyRowActions } from './resolveLegacyRowActions';
 import { RowActionMenu, formatActionLabel } from './components/RowActionMenu';
 import { BulkActionBar } from './components/BulkActionBar';
 import { BulkActionDialog } from './components/BulkActionDialog';
@@ -1446,7 +1447,16 @@ export const ObjectGrid: React.FC<ObjectGridProps> = ({
   const rowActionDefsList: any[] = Array.isArray((schema as any).rowActionDefs) ? (schema as any).rowActionDefs : [];
   const wantEditAction = rowActionsList.includes('edit');
   const wantDeleteAction = rowActionsList.includes('delete');
-  const customRowActions = rowActionsList.filter(a => a !== 'edit' && a !== 'delete');
+  // Legacy `rowActions` carry a bare action NAME, which the runner cannot
+  // execute on its own — resolve each against the object's declared actions so
+  // it dispatches as a real def (and so a name that duplicates an existing
+  // `list_item` def stops rendering a dead twin of it). See
+  // `resolveLegacyRowActions` for the full rationale (objectui#2960).
+  const { defs: resolvedRowActionDefs, unresolved: customRowActions } = resolveLegacyRowActions({
+    rowActions: rowActionsList.filter(a => a !== 'edit' && a !== 'delete'),
+    rowActionDefs: rowActionDefsList,
+    objectActions: (objectSchema as any)?.actions,
+  });
   // Honor the object's resolved CRUD affordance: the ADR-0103 lifecycle bucket
   // (`managedBy`), the `userActions.edit`/`delete` override — explicit `false`
   // opts out of the generic row Edit/Delete (e.g. sys_environment ships a
@@ -1468,7 +1478,7 @@ export const ObjectGrid: React.FC<ObjectGridProps> = ({
     effectiveApiOperations: effectiveApiOps,
   });
   const hasActions = !!(operations && (operations.update || operations.delete));
-  const hasRowActions = customRowActions.length > 0 || rowActionDefsList.length > 0 || wantEditAction || wantDeleteAction;
+  const hasRowActions = customRowActions.length > 0 || resolvedRowActionDefs.length > 0 || wantEditAction || wantDeleteAction;
 
   const columnsWithActions = (hasActions || hasRowActions) ? [
     ...persistedColumns,
@@ -1489,7 +1499,7 @@ export const ObjectGrid: React.FC<ObjectGridProps> = ({
         <RowActionMenu
           row={row}
           rowActions={customRowActions}
-          rowActionDefs={rowActionDefsList}
+          rowActionDefs={resolvedRowActionDefs as any[]}
           maxInlineActions={(schema as any).maxInlineRowActions ?? 1}
           canEdit={canEdit}
           canDelete={canDelete}

packages/plugin-grid/src/__tests__/legacyRowActionDispatch.test.tsx

@@ -0,0 +1,162 @@
+/**
+ * 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.
+ */
+
+/**
+ * Legacy string `rowActions` must reach the ActionRunner as a real action DEF
+ * (objectui#2960).
+ *
+ * A view declaring `rowActions: ['convert_lead']` used to dispatch the action
+ * NAME in the runner's `type` slot — `{ type: 'convert_lead' }`. That matches
+ * no built-in type and no registered handler, so it fell through the runner's
+ * schema fallback: zero requests, then a green "Action completed successfully"
+ * toast. And because an object action declaring `locations: ['list_item']`
+ * also injects its own (working) entry, the same action could render twice —
+ * one live, one dead.
+ *
+ * These drive the REAL ObjectGrid through a real ActionProvider and assert the
+ * user-visible outcome: the click issues the action's request, and the menu
+ * carries one entry per action rather than a working/dead pair.
+ */
+
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { render, screen, waitFor, cleanup } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import '@testing-library/jest-dom';
+import React from 'react';
+
+vi.mock('@object-ui/permissions', () => ({
+  usePermissions: () => ({ isLoaded: false, checkField: () => true, getObjectApiOperations: () => undefined }),
+}));
+
+import { ObjectGrid } from '../ObjectGrid';
+import { registerAllFields } from '@object-ui/fields';
+import { ActionProvider, SchemaRendererProvider } from '@object-ui/react';
+
+registerAllFields();
+
+const OBJECT = 'lead';
+const ROWS = [{ id: '1', name: 'Alice' }];
+
+/**
+ * The object's own `convert_lead` action — an api call to a custom route. The
+ * label is deliberately NOT the humanization of the name ("Convert Lead"), so
+ * an assertion on it can tell the resolved def apart from the legacy path,
+ * which could only ever render `formatActionLabel(name)`.
+ */
+const CONVERT_LEAD = {
+  name: 'convert_lead',
+  label: 'Convert to Account',
+  type: 'api',
+  target: '/api/v1/leads/convert',
+  locations: ['record_header'],
+};
+
+let fetchMock: ReturnType<typeof vi.fn>;
+
+function renderGrid(opts: { objectActions?: unknown[]; schema?: Record<string, unknown> }) {
+  const dataSource: any = {
+    getObjectSchema: async (name: string) => ({
+      name,
+      fields: { id: { type: 'text' }, name: { type: 'text', label: 'Name' } },
+      ...(opts.objectActions ? { actions: opts.objectActions } : {}),
+    }),
+  };
+  const schema: any = {
+    type: 'object-grid',
+    objectName: OBJECT,
+    columns: [{ field: 'name', label: 'Name' }],
+    data: { provider: 'value', items: ROWS },
+    ...opts.schema,
+  };
+  return render(
+    <ActionProvider>
+      <SchemaRendererProvider dataSource={dataSource}>
+        <ObjectGrid schema={schema} dataSource={dataSource} />
+      </SchemaRendererProvider>
+    </ActionProvider>,
+  );
+}
+
+/** Render, wait for the async schema fetch to settle, then open the row kebab. */
+async function openRowMenu(opts: Parameters<typeof renderGrid>[0]) {
+  renderGrid(opts);
+  await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument());
+  // The promotion depends on `getObjectSchema`, so an assertion taken before
+  // it lands would read the pre-fetch (unresolved) state.
+  await waitFor(() => expect(screen.getByTestId('row-action-trigger')).toBeInTheDocument());
+  await userEvent.click(screen.getByTestId('row-action-trigger'));
+}
+
+beforeEach(() => {
+  fetchMock = vi.fn().mockResolvedValue({
+    ok: true,
+    status: 200,
+    statusText: 'OK',
+    headers: { get: () => 'application/json' },
+    json: async () => ({ success: true }),
+  });
+  vi.stubGlobal('fetch', fetchMock);
+});
+
+afterEach(() => {
+  cleanup();
+  vi.unstubAllGlobals();
+});
+
+describe('legacy string rowActions dispatch (objectui#2960)', () => {
+  it('issues the resolved action request instead of no-opping', async () => {
+    await openRowMenu({
+      objectActions: [CONVERT_LEAD],
+      schema: { rowActions: ['convert_lead'] },
+    });
+
+    await userEvent.click(screen.getByTestId('row-action-convert_lead'));
+
+    await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1));
+    expect(fetchMock.mock.calls[0][0]).toBe('/api/v1/leads/convert');
+  });
+
+  it('renders the object action label, not the humanized name', async () => {
+    await openRowMenu({
+      objectActions: [CONVERT_LEAD],
+      schema: { rowActions: ['convert_lead'] },
+    });
+
+    const item = screen.getByTestId('row-action-convert_lead');
+    expect(item).toHaveTextContent('Convert to Account');
+    expect(item).not.toHaveTextContent('Convert Lead');
+  });
+
+  it('does not render a dead duplicate of a list_item action', async () => {
+    const listItemDef = { ...CONVERT_LEAD, locations: ['list_item'], label: 'Convert' };
+    await openRowMenu({
+      objectActions: [listItemDef],
+      // The app-shell derives `rowActionDefs` from `locations: ['list_item']`;
+      // a view that ALSO names the action in legacy `rowActions` used to get
+      // both a working entry and a dead twin.
+      schema: { rowActions: ['convert_lead'], rowActionDefs: [listItemDef] },
+    });
+
+    expect(screen.getAllByTestId('row-action-convert_lead')).toHaveLength(1);
+  });
+
+  it('leaves a name with no declared action to the by-name handler path', async () => {
+    // Consumers may register a runner handler under exactly this name, so the
+    // entry must still render and still dispatch.
+    await openRowMenu({
+      objectActions: [CONVERT_LEAD],
+      schema: { rowActions: ['crm_only_handler'] },
+    });
+
+    expect(screen.getByTestId('row-action-crm_only_handler')).toBeInTheDocument();
+    // Nothing resolvable to call — and with the runner's fallback now failing
+    // loudly, no request and no success are reported.
+    await userEvent.click(screen.getByTestId('row-action-crm_only_handler'));
+    await waitFor(() => expect(fetchMock).not.toHaveBeenCalled());
+  });
+});