Skip to content

Commit 4ae0ac4

Browse files
authored
fix(actions): one placement rule for locations — declare it or it renders nowhere (#3145)
Four renderers each answered "where does an action with no `locations` go?" differently: action:bar and metadata-admin showed it EVERYWHERE, page:header showed it on the header, action:group showed it for `undefined` but hid it for `[]`. ActionEngine, RecordDetailView, DeclaredActionsBar, the related-list bridge and the environment toolbar all showed it NOWHERE. The same action appeared or vanished depending on which component drew it — which is how an aggregate-only bulk action (#3139) minted a list-toolbar button whose dispatch could only fail. All of them now share one exported predicate, actionRendersAt(action, location) in @object-ui/types: an action renders at a location only if it declares that location. ADR-0078 already listed "an action with no locations" as a verified inert shape, and buildDefaultPageSchema already documented "must include locations: ['record_header'] to render". One named carve-out, now consistent across both bars: host-injected chrome (systemActions / headerSystemActions) is placed by the host, not authored, so it is never location-filtered. Selection placement stays declared by a view's bulkActions / bulkActionDefs. Authoring side so the stricter rule cannot mint invisible actions: Studio seeds locations on a new action, the inspector flags an empty placement, and the false `defaults to ['record_header']` JSDoc is gone. Closes #3142
1 parent 14f6999 commit 4ae0ac4

16 files changed

Lines changed: 391 additions & 71 deletions

File tree

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
---
2+
"@object-ui/types": minor
3+
"@object-ui/components": minor
4+
"@object-ui/app-shell": minor
5+
---
6+
7+
One placement rule for action `locations` (objectui#3142).
8+
9+
**Breaking for metadata**: an action that declares no `locations` (missing key
10+
or `[]`) no longer renders in a located surface. FROM: omitting `locations`
11+
made an action appear on the list toolbar, the record header, and every
12+
metadata-admin toolbar. TO: declare where it belongs —
13+
`locations: ['record_header']` for the record header, `['list_toolbar']` for
14+
the list toolbar, and so on. Nothing else changes; actions that already
15+
declare a location are untouched.
16+
17+
Four renderers each answered "where does an action with no `locations` go?"
18+
differently — `action:bar` and metadata-admin showed it EVERYWHERE,
19+
`page:header` showed it on the header, `action:group` showed it for
20+
`undefined` but hid it for `[]` — while `ActionEngine`, `RecordDetailView`,
21+
`DeclaredActionsBar`, the related-list bridge and the environment toolbar all
22+
showed it NOWHERE. The same action therefore appeared or vanished depending on
23+
which component happened to draw it. All eight now go through one exported
24+
predicate, `actionRendersAt(action, location)` from `@object-ui/types`: an
25+
action renders at a location only if it declares that location.
26+
27+
The strict reading is the platform's own — ADR-0078 lists "an `action` with no
28+
`locations`" as a verified inert shape, and the detail-page synthesizer already
29+
documented "must include `locations: ['record_header']` to render". The
30+
leniency contradicted both, and it is what let an aggregate-only bulk action
31+
(objectui#3139) — one with no single-record placement by construction — mint a
32+
list-toolbar button whose dispatch could only fail.
33+
34+
Two placements are declared elsewhere and need no `locations`, both unchanged:
35+
host-injected chrome in the `systemActions` / `headerSystemActions` slot (now
36+
consistently exempt on `page:header` too, where it used to be filtered), and an
37+
action named in a view's `bulkActions` / `bulkActionDefs`.
38+
39+
Authoring side: Studio seeds `locations: ['record_header']` on a new action
40+
instead of minting one that renders nowhere, and the action inspector says so
41+
when no placement is ticked. The `ActionSchema.locations` JSDoc claimed a
42+
`['record_header']` default that no renderer ever implemented — corrected.

content/docs/guide/slotted-pages.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,35 @@ Delete) as a button row. Up to **`maxVisible`** actions render inline,
103103
side by side (default **3** on desktop, **`mobileMaxVisible`**, default
104104
**1**, on mobile); the rest collapse into a `` "More actions" menu.
105105

106+
### Declaring placement
107+
108+
An authored action renders here only if its **`locations`** declares
109+
`record_header` (inline) or `record_more` (straight into the `` menu).
110+
There is no default: an action that declares no location renders in **no**
111+
located surface — not here, not the list toolbar, not the row menu. That
112+
one rule is shared by every surface that places actions by location
113+
(`action:bar`, `action:group`, `record:quick_actions`, related lists, the
114+
metadata-admin toolbars and the action engine), so an action behaves the
115+
same wherever it is drawn.
116+
117+
```ts
118+
actions: [
119+
{ name: 'convert_lead', label: 'Convert', locations: ['record_header'] },
120+
{ name: 'export_pdf', label: 'Export', locations: ['record_more'] },
121+
]
122+
```
123+
124+
Two placements come from somewhere other than `locations`, and neither
125+
needs an entry here:
126+
127+
- **Host chrome** — the Edit / Share / Delete set the host injects is
128+
placed by the host, not authored, so it is never location-filtered.
129+
- **Selection actions** — an action named in a list view's `bulkActions`
130+
or `bulkActionDefs` is placed by that declaration. An action that only
131+
makes sense over a selection (an aggregate export, say) can therefore
132+
declare no `locations` at all and still be reachable from the
133+
selection bar.
134+
106135
Which actions claim the inline slots is declared in metadata, using the
107136
same rules as `action:bar`:
108137

packages/app-shell/src/views/metadata-admin/MetadataTypeActions.test.tsx

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,42 @@ beforeEach(() => {
4343
});
4444

4545
describe('MetadataTypeActions', () => {
46+
// [#3142] These actions come off the server's `/meta/types` feed, and this
47+
// component used to carry a byte-identical copy of `action:bar`'s lenient
48+
// rule — so a type shipping an action with no `locations` got a button on
49+
// BOTH the list toolbar and the record header. Placement is declared now,
50+
// like everywhere else. Nothing pinned the lenient branch before, which is
51+
// how the two copies drifted from the rest of the repo unnoticed.
52+
it('renders only actions that declare the requested location', () => {
53+
const { container } = render(
54+
<MetadataTypeActions
55+
location="record_header"
56+
recordId="ds1"
57+
entry={{
58+
actions: [
59+
{ name: 'here', label: 'Here', type: 'api', target: '/x', locations: ['record_header'] },
60+
{ name: 'elsewhere', label: 'Elsewhere', type: 'api', target: '/x', locations: ['list_toolbar'] },
61+
{ name: 'undeclared', label: 'Undeclared', type: 'api', target: '/x' },
62+
],
63+
}}
64+
/>,
65+
);
66+
expect(screen.getByTitle('Here')).toBeTruthy();
67+
expect(screen.queryByTitle('Elsewhere')).toBeNull();
68+
expect(screen.queryByTitle('Undeclared')).toBeNull();
69+
expect(container.textContent).not.toContain('Undeclared');
70+
});
71+
72+
it('renders nothing when no action declares the requested location', () => {
73+
const { container } = render(
74+
<MetadataTypeActions
75+
location="list_toolbar"
76+
entry={{ actions: [{ name: 'undeclared', label: 'Undeclared', type: 'api', target: '/x' }] }}
77+
/>,
78+
);
79+
expect(container.firstChild).toBeNull();
80+
});
81+
4682
it('runs an api action without params directly (no dialog)', async () => {
4783
render(
4884
<MetadataTypeActions

packages/app-shell/src/views/metadata-admin/MetadataTypeActions.tsx

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import { Loader2 } from 'lucide-react';
3737
import { Button } from '@object-ui/components';
3838
import { createAuthenticatedFetch } from '@object-ui/auth';
3939
import type { ActionParamDef } from '@object-ui/core';
40+
import { actionRendersAt, type ActionLocation } from '@object-ui/types';
4041
import { getIcon } from '../../utils/getIcon';
4142
import { ActionParamDialog, type ParamDialogState } from '../ActionParamDialog';
4243
import { ActionResultDialog, type ResultDialogState } from '../ActionResultDialog';
@@ -74,8 +75,12 @@ function interpolateTarget(
7475
export interface MetadataTypeActionsProps {
7576
/** The rich type entry whose `actions` to render. */
7677
entry?: Pick<RichMetadataTypeEntry, 'actions'> | undefined;
77-
/** Which chrome slot is asking — actions are filtered by their `locations`. */
78-
location: string;
78+
/**
79+
* Which chrome slot is asking — actions are filtered by their `locations`.
80+
* Typed as the spec vocabulary rather than `string` so a typo cannot quietly
81+
* match nothing: both call sites pass a literal, so this costs them nothing.
82+
*/
83+
location: ActionLocation;
7984
/** Current item name, exposed to actions as `${ctx.recordId}`. */
8085
recordId?: string;
8186
/** Called after a successful action when `refreshAfter` is set. */
@@ -93,11 +98,12 @@ export function MetadataTypeActions({ entry, location, recordId, onAfter }: Meta
9398
const [resultState, setResultState] = React.useState<ResultDialogState>({ open: false });
9499
const authFetch = React.useMemo(() => createAuthenticatedFetch(), []);
95100

101+
// Placement is `actionRendersAt`'s call (objectui#3142). These actions come
102+
// from the server's `/meta/types` feed, so a type shipping an action with no
103+
// `locations` used to get a button on BOTH the list toolbar and the record
104+
// header; now it must declare where it belongs, like every other surface.
96105
const actions = React.useMemo(
97-
() =>
98-
(entry?.actions ?? []).filter(
99-
(a) => !a.locations || a.locations.length === 0 || a.locations.includes(location),
100-
),
106+
() => (entry?.actions ?? []).filter((a) => actionRendersAt(a, location)),
101107
[entry?.actions, location],
102108
);
103109

packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,17 @@ export function ActionDefaultInspector({
406406
<InspectorCheckboxField key={loc.value} label={loc.label} value={locations.includes(loc.value)} onCommit={(v) => toggleLocation(loc.value, v)} disabled={readOnly} />
407407
))}
408408
</div>
409+
{/* [#3142] No placement = renders in no located surface. Saying so
410+
here is the difference between an author seeing an empty list and
411+
an author hunting for a button that was never going to appear.
412+
Not an error: a selection-only action is placed by a view's
413+
`bulkActions` / `bulkActionDefs` instead, which is legitimate. */}
414+
{locations.length === 0 && (
415+
<div className="text-[11px] text-destructive">
416+
No placement selected — this action will not appear on any record or list surface. Tick a
417+
placement above, or place it from a view’s bulk actions.
418+
</div>
419+
)}
409420
<div className="grid grid-cols-2 gap-2 pt-1">
410421
<InspectorSelectField label="Component" value={str('component') || undefined} options={COMPONENT_OPTS} onCommit={(v) => onPatch({ component: v })} disabled={readOnly} />
411422
</div>

packages/app-shell/src/views/studio-design/ObjectActionsPanel.tsx

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,11 +115,19 @@ export function ObjectActionsPanel({
115115
// the user types into the body (same dead-end class as a fresh validation
116116
// rule needing a placeholder condition). The user configures the rest
117117
// (behavior/placement/…) in the form on the right.
118+
//
119+
// `locations` is seeded for the same reason (objectui#3142): an action
120+
// that declares no location renders in no located surface, so an unseeded
121+
// skeleton would save clean and then be invisible everywhere — the author
122+
// would have no button to click and nothing to tell them why. Placement
123+
// stays fully editable in the Placement checkboxes; `record_header` is
124+
// just the starting point.
118125
const fresh: ActionItem = {
119126
name,
120127
label: t('engine.studio.actions.newLabel', locale),
121128
type: 'script',
122129
objectName,
130+
locations: ['record_header'],
123131
body: { language: 'js', source: 'return { ok: true };' },
124132
};
125133
onPatch({ actions: [...actions, fresh] });

packages/components/src/__tests__/action-bar.test.tsx

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -70,18 +70,30 @@ describe('ActionBar (action:bar)', () => {
7070
expect(container.textContent).toContain('Both Action');
7171
});
7272

73-
it('shows actions without locations when filtering by location', () => {
73+
// [#3142] Deliberately INVERTED from "shows actions without locations".
74+
// An undeclared placement is not a wildcard: this bar used to show such an
75+
// action at every location, while ActionEngine, RecordDetailView,
76+
// DeclaredActionsBar and the related-list bridge all showed it at none —
77+
// so the same action appeared or vanished with the renderer. `locations`
78+
// is the declaration; no declaration, no located placement (ADR-0078 reads
79+
// an action with no `locations` as inert for exactly this reason).
80+
it('hides an action that declares no locations when filtering by location', () => {
7481
const { container } = renderComponent({
7582
type: 'action:bar',
7683
location: 'record_header',
7784
actions: [
7885
{ name: 'no_loc', label: 'No Location', type: 'script' },
79-
{ name: 'has_loc', label: 'Has Location', type: 'script', locations: ['list_toolbar'] },
86+
{ name: 'empty_loc', label: 'Empty Location', type: 'script', locations: [] },
87+
{ name: 'other_loc', label: 'Other Location', type: 'script', locations: ['list_toolbar'] },
88+
{ name: 'here_loc', label: 'Here Location', type: 'script', locations: ['record_header'] },
8089
],
8190
});
82-
// Action without locations should show in any location
83-
expect(container.textContent).toContain('No Location');
84-
expect(container.textContent).not.toContain('Has Location');
91+
expect(container.textContent).not.toContain('No Location');
92+
// An empty array reads the same as an absent one — the third dialect
93+
// (`action:group` hid `[]` but showed `undefined`) is gone too.
94+
expect(container.textContent).not.toContain('Empty Location');
95+
expect(container.textContent).not.toContain('Other Location');
96+
expect(container.textContent).toContain('Here Location');
8597
});
8698

8799
it('renders all actions when no location filter is set', () => {
@@ -400,8 +412,12 @@ describe('ActionBar (action:bar)', () => {
400412
const withUser = (user: unknown, props: { actions?: any[]; systemActions?: any[] }) =>
401413
render(<ActionProvider context={{ user } as any}><Bar {...props} /></ActionProvider>);
402414

403-
const gated = { name: 'gated', label: 'Bulk Reassign', type: 'api', requiredPermissions: ['manage_users'] };
404-
const plain = { name: 'plain', label: 'Export', type: 'api' };
415+
// Both fixtures declare the harness's location (#3142): without it the
416+
// strict placement filter would drop them before the capability gate ever
417+
// ran, and every `not.toContain` below would pass VACUOUSLY — the suite
418+
// would keep reporting green while testing nothing.
419+
const gated = { name: 'gated', label: 'Bulk Reassign', type: 'api', locations: ['list_toolbar'], requiredPermissions: ['manage_users'] };
420+
const plain = { name: 'plain', label: 'Export', type: 'api', locations: ['list_toolbar'] };
405421

406422
it('hides an action whose capability the caller lacks', () => {
407423
const { container } = withUser({ id: 'u1', systemPermissions: ['setup.access'] }, { actions: [gated, plain] });
Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
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+
* `action:group` placement (objectui#3142).
11+
*
12+
* This renderer carried a THIRD reading of `locations`: `!a.locations ||
13+
* a.locations.includes(loc)` — an action with `locations: undefined` showed,
14+
* while one with `locations: []` was hidden, for no stated reason. Both now
15+
* mean the same thing (no declared placement → not rendered here) because both
16+
* go through `actionRendersAt`. Nothing pinned this file's behaviour before,
17+
* which is how the third dialect survived.
18+
*/
19+
20+
import { describe, it, expect } from 'vitest';
21+
import { screen } from '@testing-library/react';
22+
import '@testing-library/jest-dom';
23+
import { renderComponent } from './test-utils';
24+
25+
// Action renderers are loaded by the shared vitest setup (see action-bar.test).
26+
const renderGroup = (schema: Record<string, unknown>) => renderComponent(schema as never);
27+
28+
describe('action:group — placement', () => {
29+
it('renders only actions that declare the requested location', () => {
30+
renderGroup({
31+
type: 'action:group',
32+
location: 'list_toolbar',
33+
actions: [
34+
{ name: 'here', label: 'Here', type: 'script', locations: ['list_toolbar'] },
35+
{ name: 'elsewhere', label: 'Elsewhere', type: 'script', locations: ['record_header'] },
36+
],
37+
});
38+
expect(screen.getByText('Here')).toBeInTheDocument();
39+
expect(screen.queryByText('Elsewhere')).not.toBeInTheDocument();
40+
});
41+
42+
it('treats an undeclared placement the same as an empty one — both hide', () => {
43+
renderGroup({
44+
type: 'action:group',
45+
location: 'list_toolbar',
46+
actions: [
47+
{ name: 'undeclared', label: 'Undeclared', type: 'script' },
48+
{ name: 'empty', label: 'Empty', type: 'script', locations: [] },
49+
{ name: 'here', label: 'Here', type: 'script', locations: ['list_toolbar'] },
50+
],
51+
});
52+
// Pre-#3142 `Undeclared` rendered and `Empty` did not — same key, two answers.
53+
expect(screen.queryByText('Undeclared')).not.toBeInTheDocument();
54+
expect(screen.queryByText('Empty')).not.toBeInTheDocument();
55+
expect(screen.getByText('Here')).toBeInTheDocument();
56+
});
57+
58+
it('renders every action when the group sets no location filter', () => {
59+
renderGroup({
60+
type: 'action:group',
61+
actions: [
62+
{ name: 'a', label: 'Alpha', type: 'script' },
63+
{ name: 'b', label: 'Beta', type: 'script', locations: ['record_header'] },
64+
],
65+
});
66+
expect(screen.getByText('Alpha')).toBeInTheDocument();
67+
expect(screen.getByText('Beta')).toBeInTheDocument();
68+
});
69+
});

0 commit comments

Comments
 (0)