Skip to content

Commit 6f29aa5

Browse files
os-zhuangclaude
andauthored
fix(grid): a legacy string row action runs instead of green-toasting a no-op (#2960) (#2996)
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: 1. ObjectGrid resolves legacy names against `objectDef.actions` (`resolveLegacyRowActions`). A name matching a declared action is promoted to that def and dispatched through the same path as a `list_item` action — so it actually runs, and picks up the def's label, visible/disabled predicates, param dialog and capability gate, none of which the string form could carry. A name matching an action already rendered as a def is dropped, which 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. ObjectGrid is the chokepoint for all three callers (app-shell ObjectView, plugin-view ObjectView, plugin-list ListView). 2. 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. The sibling bulk-action path carries the same `{type: <name>}` shape, but `bulkActionDefs` is never derived from `objectDef.actions`, so there is nothing to resolve against; change 2 makes it fail honestly rather than green-toast. Verified: stashing only the two source files makes all four new behavioral assertions fail — success:true where it should be false, the green toast firing, fetch called 0 times, and 2 menu entries where there should be 1. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 83e3520 commit 6f29aa5

7 files changed

Lines changed: 503 additions & 4 deletions

File tree

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

packages/core/src/actions/ActionRunner.ts

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -953,10 +953,40 @@ export class ActionRunner {
953953
};
954954
}
955955

956+
/**
957+
* Terminal fallback for a dispatch the switch above did not recognize: no
958+
* known `type`, no registered handler, no `navigate` / `api` / `endpoint` /
959+
* `onClick`. Some schema-only shapes are still runnable here — an action
960+
* whose whole payload is a `redirect`, or an explicit `reload` / `close`
961+
* directive — so this stays a real execution path.
962+
*
963+
* What it must never do is report success for an action it did not run. It
964+
* used to return `{ success: true, reload: true, close: true }` for a
965+
* COMPLETELY EMPTY schema, which is how a legacy string row action —
966+
* dispatched as `{ type: '<action name>' }`, the name landing in the `type`
967+
* slot — reached the user as a green "Action completed successfully" toast
968+
* having issued zero requests (objectui#2960). An unresolvable dispatch is a
969+
* wiring bug; surface it instead of laundering it into a success.
970+
*/
956971
private async executeActionSchema(action: ActionDef): Promise<ActionResult> {
972+
const hasApi = !!(action.api || action.endpoint);
973+
// `reload`/`close` default to true downstream, so only an EXPLICIT `true`
974+
// counts as declared intent — otherwise every empty schema would look like
975+
// it asked for a reload and slip back through this guard.
976+
const declaresBehavior =
977+
hasApi || !!action.onClick || !!action.redirect
978+
|| action.reload === true || action.close === true;
979+
if (!declaresBehavior) {
980+
const label = action.name || action.type || action.actionType || 'action';
981+
return {
982+
success: false,
983+
error: `Action "${label}" is not executable: no handler is registered under that name and it declares no api, endpoint, navigate, redirect or onClick.`,
984+
};
985+
}
986+
957987
const result: ActionResult = { success: true };
958988

959-
if (action.api || action.endpoint) {
989+
if (hasApi) {
960990
const apiResult = await this.executeAPI(action);
961991
if (!apiResult.success) return apiResult;
962992
result.data = apiResult.data;

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,57 @@ describe('ActionRunner', () => {
5555
});
5656
});
5757

58+
// ==========================================================================
59+
// Unresolvable dispatch (objectui#2960)
60+
// ==========================================================================
61+
62+
describe('unresolvable dispatch', () => {
63+
it('fails loudly when the schema declares nothing executable', async () => {
64+
// A legacy string row action reaches the runner as `{ type: <name> }` —
65+
// the NAME in the `type` slot. It matches no built-in type and no
66+
// registered handler, so there is nothing to run. This used to return
67+
// `{ success: true, reload: true, close: true }` after issuing zero
68+
// requests.
69+
const result = await runner.execute({ type: 'convert_lead', params: { record: { id: 1 } } });
70+
71+
expect(result.success).toBe(false);
72+
expect(result.error).toContain('convert_lead');
73+
expect(result.reload).toBeUndefined();
74+
});
75+
76+
it('does not toast success for an action it never ran', async () => {
77+
const toastHandler = vi.fn();
78+
runner.setToastHandler(toastHandler);
79+
80+
await runner.execute({ type: 'convert_lead' });
81+
82+
expect(toastHandler).toHaveBeenCalledTimes(1);
83+
expect(toastHandler).toHaveBeenCalledWith(
84+
expect.stringContaining('convert_lead'),
85+
expect.objectContaining({ type: 'error' }),
86+
);
87+
});
88+
89+
it('still runs a handler registered under the action name', async () => {
90+
// The by-name dispatch path is how consumers wire custom row actions;
91+
// the guard must not close it.
92+
const handler = vi.fn().mockResolvedValue({ success: true });
93+
runner.registerHandler('convert_lead', handler);
94+
95+
const result = await runner.execute({ type: 'convert_lead' });
96+
97+
expect(result.success).toBe(true);
98+
expect(handler).toHaveBeenCalledOnce();
99+
});
100+
101+
it('still runs a typeless action that only declares a redirect', async () => {
102+
const result = await runner.execute({ redirect: '/leads/1' });
103+
104+
expect(result.success).toBe(true);
105+
expect(result.redirect).toBe('/leads/1');
106+
});
107+
});
108+
58109
// ==========================================================================
59110
// Conditions and disabled
60111
// ==========================================================================

packages/plugin-grid/src/ObjectGrid.tsx

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ import { useGroupedData } from './useGroupedData';
4444
import { GroupRow } from './GroupRow';
4545
import { useColumnSummary } from './useColumnSummary';
4646
import { resolveRowCrudAffordances } from './rowCrudAffordances';
47+
import { resolveLegacyRowActions } from './resolveLegacyRowActions';
4748
import { RowActionMenu, formatActionLabel } from './components/RowActionMenu';
4849
import { BulkActionBar } from './components/BulkActionBar';
4950
import { BulkActionDialog } from './components/BulkActionDialog';
@@ -1446,7 +1447,16 @@ export const ObjectGrid: React.FC<ObjectGridProps> = ({
14461447
const rowActionDefsList: any[] = Array.isArray((schema as any).rowActionDefs) ? (schema as any).rowActionDefs : [];
14471448
const wantEditAction = rowActionsList.includes('edit');
14481449
const wantDeleteAction = rowActionsList.includes('delete');
1449-
const customRowActions = rowActionsList.filter(a => a !== 'edit' && a !== 'delete');
1450+
// Legacy `rowActions` carry a bare action NAME, which the runner cannot
1451+
// execute on its own — resolve each against the object's declared actions so
1452+
// it dispatches as a real def (and so a name that duplicates an existing
1453+
// `list_item` def stops rendering a dead twin of it). See
1454+
// `resolveLegacyRowActions` for the full rationale (objectui#2960).
1455+
const { defs: resolvedRowActionDefs, unresolved: customRowActions } = resolveLegacyRowActions({
1456+
rowActions: rowActionsList.filter(a => a !== 'edit' && a !== 'delete'),
1457+
rowActionDefs: rowActionDefsList,
1458+
objectActions: (objectSchema as any)?.actions,
1459+
});
14501460
// Honor the object's resolved CRUD affordance: the ADR-0103 lifecycle bucket
14511461
// (`managedBy`), the `userActions.edit`/`delete` override — explicit `false`
14521462
// opts out of the generic row Edit/Delete (e.g. sys_environment ships a
@@ -1468,7 +1478,7 @@ export const ObjectGrid: React.FC<ObjectGridProps> = ({
14681478
effectiveApiOperations: effectiveApiOps,
14691479
});
14701480
const hasActions = !!(operations && (operations.update || operations.delete));
1471-
const hasRowActions = customRowActions.length > 0 || rowActionDefsList.length > 0 || wantEditAction || wantDeleteAction;
1481+
const hasRowActions = customRowActions.length > 0 || resolvedRowActionDefs.length > 0 || wantEditAction || wantDeleteAction;
14721482

14731483
const columnsWithActions = (hasActions || hasRowActions) ? [
14741484
...persistedColumns,
@@ -1489,7 +1499,7 @@ export const ObjectGrid: React.FC<ObjectGridProps> = ({
14891499
<RowActionMenu
14901500
row={row}
14911501
rowActions={customRowActions}
1492-
rowActionDefs={rowActionDefsList}
1502+
rowActionDefs={resolvedRowActionDefs as any[]}
14931503
maxInlineActions={(schema as any).maxInlineRowActions ?? 1}
14941504
canEdit={canEdit}
14951505
canDelete={canDelete}
Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* Legacy string `rowActions` must reach the ActionRunner as a real action DEF
11+
* (objectui#2960).
12+
*
13+
* A view declaring `rowActions: ['convert_lead']` used to dispatch the action
14+
* NAME in the runner's `type` slot — `{ type: 'convert_lead' }`. That matches
15+
* no built-in type and no registered handler, so it fell through the runner's
16+
* schema fallback: zero requests, then a green "Action completed successfully"
17+
* toast. And because an object action declaring `locations: ['list_item']`
18+
* also injects its own (working) entry, the same action could render twice —
19+
* one live, one dead.
20+
*
21+
* These drive the REAL ObjectGrid through a real ActionProvider and assert the
22+
* user-visible outcome: the click issues the action's request, and the menu
23+
* carries one entry per action rather than a working/dead pair.
24+
*/
25+
26+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
27+
import { render, screen, waitFor, cleanup } from '@testing-library/react';
28+
import userEvent from '@testing-library/user-event';
29+
import '@testing-library/jest-dom';
30+
import React from 'react';
31+
32+
vi.mock('@object-ui/permissions', () => ({
33+
usePermissions: () => ({ isLoaded: false, checkField: () => true, getObjectApiOperations: () => undefined }),
34+
}));
35+
36+
import { ObjectGrid } from '../ObjectGrid';
37+
import { registerAllFields } from '@object-ui/fields';
38+
import { ActionProvider, SchemaRendererProvider } from '@object-ui/react';
39+
40+
registerAllFields();
41+
42+
const OBJECT = 'lead';
43+
const ROWS = [{ id: '1', name: 'Alice' }];
44+
45+
/**
46+
* The object's own `convert_lead` action — an api call to a custom route. The
47+
* label is deliberately NOT the humanization of the name ("Convert Lead"), so
48+
* an assertion on it can tell the resolved def apart from the legacy path,
49+
* which could only ever render `formatActionLabel(name)`.
50+
*/
51+
const CONVERT_LEAD = {
52+
name: 'convert_lead',
53+
label: 'Convert to Account',
54+
type: 'api',
55+
target: '/api/v1/leads/convert',
56+
locations: ['record_header'],
57+
};
58+
59+
let fetchMock: ReturnType<typeof vi.fn>;
60+
61+
function renderGrid(opts: { objectActions?: unknown[]; schema?: Record<string, unknown> }) {
62+
const dataSource: any = {
63+
getObjectSchema: async (name: string) => ({
64+
name,
65+
fields: { id: { type: 'text' }, name: { type: 'text', label: 'Name' } },
66+
...(opts.objectActions ? { actions: opts.objectActions } : {}),
67+
}),
68+
};
69+
const schema: any = {
70+
type: 'object-grid',
71+
objectName: OBJECT,
72+
columns: [{ field: 'name', label: 'Name' }],
73+
data: { provider: 'value', items: ROWS },
74+
...opts.schema,
75+
};
76+
return render(
77+
<ActionProvider>
78+
<SchemaRendererProvider dataSource={dataSource}>
79+
<ObjectGrid schema={schema} dataSource={dataSource} />
80+
</SchemaRendererProvider>
81+
</ActionProvider>,
82+
);
83+
}
84+
85+
/** Render, wait for the async schema fetch to settle, then open the row kebab. */
86+
async function openRowMenu(opts: Parameters<typeof renderGrid>[0]) {
87+
renderGrid(opts);
88+
await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument());
89+
// The promotion depends on `getObjectSchema`, so an assertion taken before
90+
// it lands would read the pre-fetch (unresolved) state.
91+
await waitFor(() => expect(screen.getByTestId('row-action-trigger')).toBeInTheDocument());
92+
await userEvent.click(screen.getByTestId('row-action-trigger'));
93+
}
94+
95+
beforeEach(() => {
96+
fetchMock = vi.fn().mockResolvedValue({
97+
ok: true,
98+
status: 200,
99+
statusText: 'OK',
100+
headers: { get: () => 'application/json' },
101+
json: async () => ({ success: true }),
102+
});
103+
vi.stubGlobal('fetch', fetchMock);
104+
});
105+
106+
afterEach(() => {
107+
cleanup();
108+
vi.unstubAllGlobals();
109+
});
110+
111+
describe('legacy string rowActions dispatch (objectui#2960)', () => {
112+
it('issues the resolved action request instead of no-opping', async () => {
113+
await openRowMenu({
114+
objectActions: [CONVERT_LEAD],
115+
schema: { rowActions: ['convert_lead'] },
116+
});
117+
118+
await userEvent.click(screen.getByTestId('row-action-convert_lead'));
119+
120+
await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1));
121+
expect(fetchMock.mock.calls[0][0]).toBe('/api/v1/leads/convert');
122+
});
123+
124+
it('renders the object action label, not the humanized name', async () => {
125+
await openRowMenu({
126+
objectActions: [CONVERT_LEAD],
127+
schema: { rowActions: ['convert_lead'] },
128+
});
129+
130+
const item = screen.getByTestId('row-action-convert_lead');
131+
expect(item).toHaveTextContent('Convert to Account');
132+
expect(item).not.toHaveTextContent('Convert Lead');
133+
});
134+
135+
it('does not render a dead duplicate of a list_item action', async () => {
136+
const listItemDef = { ...CONVERT_LEAD, locations: ['list_item'], label: 'Convert' };
137+
await openRowMenu({
138+
objectActions: [listItemDef],
139+
// The app-shell derives `rowActionDefs` from `locations: ['list_item']`;
140+
// a view that ALSO names the action in legacy `rowActions` used to get
141+
// both a working entry and a dead twin.
142+
schema: { rowActions: ['convert_lead'], rowActionDefs: [listItemDef] },
143+
});
144+
145+
expect(screen.getAllByTestId('row-action-convert_lead')).toHaveLength(1);
146+
});
147+
148+
it('leaves a name with no declared action to the by-name handler path', async () => {
149+
// Consumers may register a runner handler under exactly this name, so the
150+
// entry must still render and still dispatch.
151+
await openRowMenu({
152+
objectActions: [CONVERT_LEAD],
153+
schema: { rowActions: ['crm_only_handler'] },
154+
});
155+
156+
expect(screen.getByTestId('row-action-crm_only_handler')).toBeInTheDocument();
157+
// Nothing resolvable to call — and with the runner's fallback now failing
158+
// loudly, no request and no success are reported.
159+
await userEvent.click(screen.getByTestId('row-action-crm_only_handler'));
160+
await waitFor(() => expect(fetchMock).not.toHaveBeenCalled());
161+
});
162+
});

0 commit comments

Comments
 (0)