Skip to content

Commit 0bbb67b

Browse files
refactor(app-shell): Explain access picks its object and operation with the shared Select (objectui#11865) (#11990)
Part of #11865 Clause-②: no The *Explain access* slice of objectui#11865 (claim 6063378622): the panel's two browser-native select elements, *Object* and *Operation*, become the shared Radix `Select`. The card stays open for the other native selects its body counts. The earlier slices are objectui#11934 (the Group panel) and objectui#11976 (the validation editor). ## What changed - `packages/app-shell/src/views/metadata-admin/AccessExplainPanel.tsx`: one module-private `ExplainPicker`, drawn with the shared `Select` from `@object-ui/components`, renders both pickers. It follows the validation editor's `RulePicker`: items carry their option's index, so "Select an object…" (value `''`, which `SelectItem` refuses) needs no stand-in value, and a value none of the options carries gets an item of its own. The trigger carries the control's `id`, so each label's `htmlFor` still names it. - Untouched: the free-text `Input` fallback when the package lists no objects, the user picker, the record selector, the explain request, the props and the exports. No i18n key is added; the panel's set of `engine.*` keys is identical to the base's (diffed). - `AccessExplainPanel.sharedSelect-11865.test.tsx` (new), and `AccessExplainPanel.test.tsx`, whose package-scoped dropdown test now opens and picks through the trigger. It still checks the same three things: the options listed, the free-text placeholder gone, and the object the request sends. - `.changeset/11865-explain-access-shared-select.md`: `@object-ui/app-shell: patch`. ## Why Select and not Combobox for the object list Triage allows `Combobox` where a list is long, and a package's object list is open-ended, so it was built first and measured. Both readings come from Chromium, on a scratch page that mounted this panel with a package of 25 objects. The scratch page was deleted and never committed. - **Shared `Combobox`** (built, then kept out of the branch): a 400 px mouse wheel over its open list left `scrollTop` at 0, with the list's `scrollHeight` 840 and `clientHeight` 300. The panel lives in a `Sheet`, and there the list did not scroll under the wheel. - **Shared `Select`**: the same wheel moved its viewport from 0 to 208, the maximum for `scrollHeight` 840 and `clientHeight` 632. Typing `showcase_t` in the open list jumped to `showcase_task`. The `Combobox` also reports a re-pick of its current option as `''`. So the object list is a `Select`, like the operation list. ## Dispatch hypotheses - **H1, value parity: held.** The base panel was read first with a scratch probe (not committed): a `change` on each native option, then Explain. The new suite pins every option of both pickers against the request body read there: - each object sends its name; - each of the seven operations sends its name; - "Select an object…" leaves Explain disabled, as on the base; - re-picking the current option sends the same request. One caveat: the probe's `change` event fires even for the current option, so the re-pick rows pin the primitive's behaviour, not a browser reading of the native one. - **H2, labels and keyboard: held.** - `getByRole('combobox', { name: 'Object' })` and `{ name: 'Operation' }` find the two triggers (the base probe found the native controls by the same names). - Enter opens each picker and Enter on an option selects it. This is pinned in the test DOM and was also seen in Chromium. - The panel has no disabled or read-only state. It takes no such prop, its one mount (`StudioDesignSurface`) passes `open`, `onOpenChange` and `packageId`, and it writes no metadata. So objectui#11781 has nothing to reach here. - **H3, a value outside the options: held.** - On the base, a held object the package does not list (`crm_lead`) showed "Select an object…" (`selectedIndex` 0), while Explain sent `crm_lead`. - Now the trigger shows `crm_lead`, and it is listed first. Picking it changes nothing, and Explain still sends `crm_lead`. - The pin reaches that state through `defaultObject`. - **H4, first-load bytes: measured, −1 gzip byte.** - Method: `apps/console` `vite build`, then `node scripts/check-eager-closure-budget.mjs`, on this head and on the same tree with the panel at its base blob `5aade6a7`, the only bundle input the branch changes. - Eager gzip bytes: 3390584 on head and 3390585 on base. Eager raw bytes are equal (11491936), with the same 337 of 2459 eager chunks. - The only eager chunk that moves is the entry `index` chunk: 0 raw bytes, −1 gzip byte. - The panel ships in the lazy `studioBuilder` chunk on both legs. - The gate prints the same line on both legs: "Console eager closure is 3311.1 KB gzipped across 337 of 2459 chunks (budget: 3312.0 KB, headroom: 0.9 KB)". - These are local builds. The budget bot's reading on this PR is the one that counts. ## Tests and gates, on head `0c5300d7c` - Repo-root `pnpm exec vitest run --maxWorkers=2` over the new suite, `AccessExplainPanel.test.tsx` and `AccessExplainPanel.principalPerson-11862.test.tsx`, the only suites that mount the real panel (the `StudioDesignSurface` suites stub it): "Test Files 3 passed (3)", "Tests 28 passed (28)". - The two repo-wide pins, `packages/core/src/utils/__tests__/column-identity.ratchet.test.ts` and `scripts/__tests__/one-authority-per-exported-name-6273.test.ts`: "Test Files 2 passed (2)", "Tests 18 passed (18)". - `pnpm --filter @object-ui/app-shell type-check` (echoes `tsc --noEmit && tsc -p tsconfig.test.json`) exits 0. It ran after the `@object-ui/app-shell^...` closure build (29 projects, exit 0). `--listFilesOnly` shows the panel in the main program and all three panel suites in the test program. - `pnpm exec eslint` on the three touched files exits 0. The JSON format reports 3 files, 0 errors and 8 warnings. The base blobs of the two existing files report the same 8 (`no-explicit-any` and `set-state-in-effect` in untouched code), and the new suite reports none. - All exit 0: - `node scripts/check-changeset-presence.mjs` ("3 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)"); - `node scripts/check-changeset-no-major.mjs`; - `pnpm check:new-line-citations` ("0 new citation(s)"); - `pnpm check:control-bytes`; - `check-changeset-fixed`, `check-changeset-overwrite`, `check-changeset-claims`; - `check:pending-changeset-literals`, `check:test-path-roots`; - `check:vi-mock-specifiers`, `check:vi-mock-inherit`, `check:vi-mock-override-shape`; - `check:phantom-deps`, `check:unreferenced-sources`, `check:i18n-keys`, `check:i18n-designer-parity`, `check:metadata-write-doors`. - Declared to CI: the full `pnpm test` farm and repo-wide `pnpm lint`. **Reverse leg.** The implementation was committed first (`a551c491f`). The panel was then written from its base blob, and the landing was proven: 2 native select elements, 0 `ExplainPicker`, and a hash equal to the base blob. The new suite and `AccessExplainPanel.test.tsx` then read "Tests 18 failed | 5 passed (23)": - red: all 17 new pins, and the package-scoped dropdown test; - green: the other five existing tests. That is the direction the suite header predicts. The trap restored the file with `git checkout HEAD --`. The restore was proven: the hash equals the HEAD blob and `git diff HEAD` is 0 bytes. ## Acceptance notes - **The shared `Combobox` inside a `Sheet`.** Its open list did not scroll under the mouse wheel, measured once in Chromium on the scratch page above. This bears on this card's own remaining work: a later slice that moves a long list inside a dialog or sheet to `Combobox` would meet it. It is not filed from here, because no public entry on `main` was measured. `ObjectRefField` and `RecipientPickerField` import the `Combobox`, and whether either one renders inside a modal was not read. - **The `Combobox`'s search reads the option value only.** It passes no keywords to cmdk, so an option's label (a translated object label, for example) is not searchable. This was read from source and not measured. - **`defaultObject` has no caller in the tree.** In the tree, the outside-the-options state is reached by text typed into the free-text input before the package's object list arrives (read from source, not measured). --- _Generated by [Claude Code](https://claude.ai/code/session_01MgfduSkFrfM3eorB3UGfAU)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 74add0c commit 0bbb67b

4 files changed

Lines changed: 334 additions & 26 deletions

File tree

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
'@object-ui/app-shell': patch
3+
---
4+
5+
Studio's *Explain access* panel picks the object and the operation with the shared `Select`, the control Studio's validation rule editor already picks with (objectui#11865, the *Explain access* part of that card).
6+
7+
The two pickers were browser-native selects, so they looked and behaved differently from Studio's other dropdowns. They now use the shared Radix `Select`: the same trigger, dropdown and keyboard behaviour. In the object list, typing an object's leading letters jumps to it.
8+
9+
What they set is unchanged. Each option makes Explain send the same request as before: an object sends its name, an operation sends its name, and "Select an object…" leaves Explain disabled until an object is picked. Re-picking the current option changes nothing. Each picker keeps the accessible name its label gave the native select.
10+
11+
One display change: when the object the panel holds is not one of the package's objects, the object picker now shows that object. The native select showed "Select an object…" instead, while Explain still asked about the held object.
12+
13+
Unchanged: without a package's object list the object is still a free-text input, and the user picker, the record selector and the explain request itself are untouched.
14+
15+
**Clause-②: no.** No published face moves: `AccessExplainPanel` takes the same props, the package entry exports the same names, and no i18n key is added. What moves is the panel's own markup, described above.
Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,213 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* *Explain access* picks its object and its operation with the shared `Select`
5+
* (objectui#11865).
6+
*
7+
* The request form picked the object (a package's objects, plus "select an
8+
* object") and the operation with browser-native selects, beside the shared
9+
* Radix `Select` the rest of Studio picks with. The card asks for one control
10+
* for one kind of choice, surface by surface; this suite covers this panel's
11+
* two pickers.
12+
*
13+
* What is pinned:
14+
* - each picker IS the primitive (a Radix combobox trigger), keeps the name
15+
* its `<label htmlFor>` gave the native control, and no native select is
16+
* left;
17+
* - every option of both pickers sets what the native control set, read
18+
* back as the explain request the panel then sends: the JSON body, or a
19+
* disabled Explain button for "select an object" (the object is `''`);
20+
* - re-picking the current option sets nothing;
21+
* - an object the package does not list is what the object trigger shows;
22+
* - the keyboard alone opens a picker and selects.
23+
*
24+
* Not pinned, because the panel has none: a disabled or read-only state. The
25+
* panel asks a question and writes no metadata, so a read-only package
26+
* renders the same form (objectui#11781 has no reach here).
27+
*
28+
* DIRECTION, observed against the native controls: every pin here is red
29+
* there, because each one opens the primitive's listbox or reads its trigger.
30+
* What makes the request pins guards of "the conversion changed nothing the
31+
* form sends" is the literal each compares against: a `change` event on the
32+
* pre-conversion panel's native control, followed by Explain, sent that same
33+
* body, read once on that component.
34+
*/
35+
36+
import '@testing-library/jest-dom/vitest';
37+
import { afterEach, describe, expect, it, vi } from 'vitest';
38+
import { cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react';
39+
40+
const fetchSpy = vi.fn();
41+
// The package's objects, as the metadata client lists them; unsorted, as a
42+
// server may send them. A `mock` prefix so the hoisted factory may close over it.
43+
const mockObjectList = [
44+
{ item: { name: 'showcase_task' } },
45+
{ item: { name: 'showcase_account', label: 'Account' } },
46+
{ item: { name: 'showcase_project', label: 'showcase_project' } },
47+
];
48+
// One client for every render, so the panel's list effect runs once per open.
49+
const mockClient = { list: async () => mockObjectList };
50+
51+
vi.mock('@object-ui/auth', async (importOriginal) => ({
52+
...(await importOriginal<Record<string, unknown>>()),
53+
createAuthenticatedFetch: () => fetchSpy,
54+
}));
55+
vi.mock('@object-ui/react', async (importOriginal) => ({
56+
...(await importOriginal<Record<string, unknown>>()),
57+
useAdapter: () => ({ find: vi.fn(async () => []) }),
58+
}));
59+
vi.mock('./useMetadata', () => ({
60+
useMetadataClient: () => mockClient,
61+
}));
62+
// The record and user pickers have their own coverage.
63+
vi.mock('@object-ui/fields', async (importOriginal) => ({
64+
...(await importOriginal<typeof import('@object-ui/fields')>()),
65+
RecordPickerDialog: () => null,
66+
}));
67+
68+
import { AccessExplainPanel } from './AccessExplainPanel';
69+
70+
afterEach(() => {
71+
cleanup();
72+
fetchSpy.mockReset();
73+
});
74+
75+
const DECISION = {
76+
allowed: true,
77+
object: 'showcase_task',
78+
operation: 'read',
79+
principal: { userId: 'u_1', positions: [], permissionSets: [] },
80+
layers: [{ layer: 'object_crud', verdict: 'grants', detail: 'granted', contributors: [] }],
81+
};
82+
83+
/**
84+
* Mount the panel scoped to a package, starting on `defaultObject` (`null`: on
85+
* no object), and wait for the object picker to replace the free-text input.
86+
*/
87+
async function mount(defaultObject: string | null = 'showcase_task'): Promise<void> {
88+
render(
89+
<AccessExplainPanel
90+
open
91+
onOpenChange={() => {}}
92+
packageId="com.example.showcase"
93+
defaultObject={defaultObject ?? undefined}
94+
/>,
95+
);
96+
await waitFor(() => expect(screen.getByLabelText('Object')).toHaveAttribute('role', 'combobox'));
97+
}
98+
99+
const trigger = (name: 'Object' | 'Operation') => screen.getByRole('combobox', { name });
100+
101+
/** Open a picker from the keyboard and return the options it lists, in order. */
102+
async function openPicker(name: 'Object' | 'Operation'): Promise<HTMLElement[]> {
103+
fireEvent.keyDown(trigger(name), { key: 'ArrowDown' });
104+
const listbox = await screen.findByRole('listbox');
105+
return within(listbox).getAllByRole('option');
106+
}
107+
108+
async function pick(name: 'Object' | 'Operation', label: string): Promise<void> {
109+
const options = await openPicker(name);
110+
const option = options.find((o) => o.textContent === label);
111+
if (!option) throw new Error(`${name} lists no "${label}": ${options.map((o) => o.textContent).join(' | ')}`);
112+
fireEvent.click(option);
113+
await waitFor(() => expect(screen.queryByRole('listbox')).toBeNull());
114+
}
115+
116+
/** What Explain sends now: the request body, or `DISABLED` when the button cannot be pressed. */
117+
async function explainBody(): Promise<string> {
118+
const button = screen.getByRole('button', { name: /explain$/i });
119+
if ((button as HTMLButtonElement).disabled) return 'DISABLED';
120+
fetchSpy.mockResolvedValue({ ok: true, status: 200, statusText: '', json: async () => DECISION });
121+
fireEvent.click(button);
122+
await screen.findByTestId('explain-verdict');
123+
expect(fetchSpy).toHaveBeenCalledTimes(1);
124+
return String(fetchSpy.mock.calls[0][1].body);
125+
}
126+
127+
describe('Explain access pickers are the shared Select (objectui#11865)', () => {
128+
it('renders each picker as the Radix combobox trigger, named by its label, with no native select left', async () => {
129+
await mount();
130+
expect(document.querySelector('select')).toBeNull();
131+
132+
for (const [name, id, shown] of [
133+
['Object', 'explain-object', 'showcase_task'],
134+
['Operation', 'explain-operation', 'Read'],
135+
] as const) {
136+
const el = trigger(name);
137+
expect(el.tagName).toBe('BUTTON');
138+
expect(el).toHaveAttribute('id', id);
139+
expect(el).toHaveTextContent(shown);
140+
}
141+
});
142+
143+
it('opens on "select an object" when nothing is chosen, and Explain waits for an object', async () => {
144+
await mount(null);
145+
expect(trigger('Object')).toHaveTextContent('Select an object…');
146+
expect(await explainBody()).toBe('DISABLED');
147+
});
148+
});
149+
150+
/**
151+
* [picker, option label, what Explain sends after the pick]. The panel starts
152+
* on `showcase_task` / `Read`, so the `showcase_task` and `Read` rows re-pick
153+
* the current option, and set nothing.
154+
*/
155+
const PICKS: ReadonlyArray<readonly ['Object' | 'Operation', string, string]> = [
156+
['Object', 'Select an object…', 'DISABLED'],
157+
['Object', 'Account (showcase_account)', '{"object":"showcase_account","operation":"read"}'],
158+
['Object', 'showcase_project', '{"object":"showcase_project","operation":"read"}'],
159+
['Object', 'showcase_task', '{"object":"showcase_task","operation":"read"}'],
160+
['Operation', 'Read', '{"object":"showcase_task","operation":"read"}'],
161+
['Operation', 'Create', '{"object":"showcase_task","operation":"create"}'],
162+
['Operation', 'Update', '{"object":"showcase_task","operation":"update"}'],
163+
['Operation', 'Delete', '{"object":"showcase_task","operation":"delete"}'],
164+
['Operation', 'Transfer', '{"object":"showcase_task","operation":"transfer"}'],
165+
['Operation', 'Restore', '{"object":"showcase_task","operation":"restore"}'],
166+
['Operation', 'Purge', '{"object":"showcase_task","operation":"purge"}'],
167+
];
168+
169+
describe('every option sets what the native control set', () => {
170+
it('the table covers every option of both pickers, in the order each lists them', async () => {
171+
for (const name of ['Object', 'Operation'] as const) {
172+
await mount();
173+
const listed = (await openPicker(name)).map((o) => o.textContent);
174+
expect(listed, name).toEqual(PICKS.filter(([p]) => p === name).map(([, label]) => label));
175+
cleanup();
176+
}
177+
});
178+
179+
it.each(PICKS.map((row) => [`${row[0]} → ${row[1]}`, ...row] as const))('%s', async (_name, name, label, sent) => {
180+
await mount();
181+
await pick(name, label);
182+
expect(trigger(name)).toHaveTextContent(label);
183+
expect(await explainBody()).toBe(sent);
184+
});
185+
});
186+
187+
describe('an object the package does not list is what the trigger shows', () => {
188+
it('shown, listed first, and re-picking it sets nothing', async () => {
189+
await mount('crm_lead');
190+
// The native control showed "Select an object…" here, while Explain asked about crm_lead.
191+
expect(trigger('Object')).toHaveTextContent('crm_lead');
192+
const listed = (await openPicker('Object')).map((o) => o.textContent);
193+
expect(listed).toEqual(['crm_lead', 'Select an object…', 'Account (showcase_account)', 'showcase_project', 'showcase_task']);
194+
fireEvent.click(screen.getAllByRole('option')[0]);
195+
await waitFor(() => expect(screen.queryByRole('listbox')).toBeNull());
196+
expect(await explainBody()).toBe('{"object":"crm_lead","operation":"read"}');
197+
});
198+
});
199+
200+
describe('the keyboard alone picks', () => {
201+
it.each([
202+
['Object', 'showcase_project', '{"object":"showcase_project","operation":"read"}'],
203+
['Operation', 'Delete', '{"object":"showcase_task","operation":"delete"}'],
204+
] as const)('%s: Enter opens it and Enter on an option selects it', async (name, label, sent) => {
205+
await mount();
206+
fireEvent.keyDown(trigger(name), { key: 'Enter' });
207+
const listbox = await screen.findByRole('listbox');
208+
fireEvent.keyDown(within(listbox).getByRole('option', { name: label }), { key: 'Enter' });
209+
await waitFor(() => expect(screen.queryByRole('listbox')).toBeNull());
210+
expect(trigger(name)).toHaveTextContent(label);
211+
expect(await explainBody()).toBe(sent);
212+
});
213+
});

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

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -201,17 +201,21 @@ describe('AccessExplainPanel (ADR-0090 D6)', () => {
201201
];
202202
render(<AccessExplainPanel open onOpenChange={() => {}} packageId="com.example.showcase" />);
203203

204-
// The object field swaps from a free-text input to a <select> once loaded.
205-
await waitFor(() => expect(screen.getByLabelText('Object').tagName).toBe('SELECT'));
204+
// The object field swaps from a free-text input to the shared Select's
205+
// trigger once loaded (objectui#11865).
206+
await waitFor(() => expect(screen.getByLabelText('Object')).toHaveAttribute('role', 'combobox'));
206207
const objectField = screen.getByLabelText('Object');
207-
expect(within(objectField).getByRole('option', { name: 'Account (showcase_account)' })).toBeInTheDocument();
208-
expect(within(objectField).getByRole('option', { name: 'showcase_task' })).toBeInTheDocument();
208+
fireEvent.keyDown(objectField, { key: 'ArrowDown' });
209+
const listbox = await screen.findByRole('listbox');
210+
expect(within(listbox).getByRole('option', { name: 'Account (showcase_account)' })).toBeInTheDocument();
211+
expect(within(listbox).getByRole('option', { name: 'showcase_task' })).toBeInTheDocument();
209212
// The misleading free-text placeholder example is gone.
210213
expect(screen.queryByPlaceholderText(/crm_lead/)).toBeNull();
211214

212215
// Selecting an object and explaining posts that object.
213216
fetchSpy.mockResolvedValue(jsonResponse(200, DECISION));
214-
fireEvent.change(objectField, { target: { value: 'showcase_account' } });
217+
fireEvent.click(within(listbox).getByRole('option', { name: 'Account (showcase_account)' }));
218+
await waitFor(() => expect(screen.queryByRole('listbox')).toBeNull());
215219
fireEvent.click(screen.getByRole('button', { name: /explain$/i }));
216220
await waitFor(() => expect(fetchSpy).toHaveBeenCalled());
217221
const body = JSON.parse(String((fetchSpy.mock.calls[0]?.[1] as any)?.body ?? '{}'));

0 commit comments

Comments
 (0)