From c17557f14597340ea2660e160774f4a603b99a95 Mon Sep 17 00:00:00 2001 From: Lars George Date: Fri, 22 May 2026 13:33:17 +0200 Subject: [PATCH] fix(directory): PrincipalPicker UI polish + Radix Dialog pointer-events bug Stacks on Phase 4 (#417). Post-merge polish surfaced during manual QA against the file-backed directory provider. Pointer-events (root cause): - Radix Dialog locks ``pointer-events: none`` on while open. The picker's type-ahead Popover is portaled to and therefore inherited the lock, making every result row unclickable inside any Dialog or Sheet host (Role form, Team form, Owner dialog, DC wizard, workflow designer Sheet, comment sidebar Sheet, etc.). Clicks landed on the dialog's own help-text

instead. Verified end-to-end via Playwright (elementsFromPoint returned the dialog subtree). - Fix: explicit ``style={{ pointerEvents: 'auto' }}`` on the picker's PopoverContent so the portaled popover subtree overrides the body lock. One-line change inside PrincipalPicker covers all 18 call sites without touching any consumer. Browse sub-dialog UX: - Always close after a pick, even in multi-select. Browse is the discovery path; the inline type-ahead is the fast path for adding several entries in succession. Avoids the previous confusing "added but the dialog stayed open" behaviour. - Reset query + filter when the dialog re-opens (via useEffect on ``open``), not on the close path. Several close paths (Close button, parent state change, single-select auto-close) bypassed the prior onOpenChange-based reset, so re-opening showed stale search text. Inline type-ahead focus: - onMouseDown={preventDefault} on PrincipalRow keeps focus on the input so the popover's dismiss-on-blur doesn't race the click handler. Defensive even with the pointer-events fix in place. Layout + labels: - SelectionBadges now render below the input row (was inline). Pills can grow tall on multi-select; below-input layout matches Gmail- style chip lists and keeps the input width stable. - Drop the stale "(comma-separated)" suffix from the Assigned Directory Groups label. With the directory configured the badges are discrete, not comma-separated, so the hint was misleading. Self-rendered manual-mode hint inside the picker shows "Press Enter, Tab, or comma to add an entry." only when the directory is unconfigured. i18n updated across en, de, es, fr, it, ja, nl. Tests: - principal-picker.test.tsx: 19 passed (was 13). New cases cover the badge-below-input order, dialog reset-on-open, manual-mode hint visibility, close-after-pick in multi-select, and a regression test asserting row mousedown is default-prevented. Verified live via Playwright: inline pick (Edit Role -> "pro" -> Data Producers added as badge, input cleared); Browse dialog pick (search "con" -> Data Consumers added, dialog closed); reset-on- reopen confirmed empty input after closing with text and reopening. --- .../common/principal-picker.test.tsx | 125 ++++++++++++++++++ .../components/common/principal-picker.tsx | 53 ++++++-- .../src/i18n/locales/de/settings.json | 2 +- .../src/i18n/locales/en/settings.json | 2 +- .../src/i18n/locales/es/settings.json | 2 +- .../src/i18n/locales/fr/settings.json | 2 +- .../src/i18n/locales/it/settings.json | 2 +- .../src/i18n/locales/ja/settings.json | 2 +- .../src/i18n/locales/nl/settings.json | 2 +- 9 files changed, 173 insertions(+), 19 deletions(-) diff --git a/src/frontend/src/components/common/principal-picker.test.tsx b/src/frontend/src/components/common/principal-picker.test.tsx index 88d028d3..13495ba9 100644 --- a/src/frontend/src/components/common/principal-picker.test.tsx +++ b/src/frontend/src/components/common/principal-picker.test.tsx @@ -254,3 +254,128 @@ describe('PrincipalPicker — configured mode', () => { expect(onChange).toHaveBeenLastCalledWith(['alice@x.com']); }); }); + +describe('PrincipalPicker — manual-entry hint', () => { + it('renders the manual-mode hint only when directory is unconfigured', () => { + setDirectoryStatus(false); + const { rerender } = renderWithProviders( + {}} />, + ); + expect(screen.getByTestId('principal-picker-manual-hint')).toBeInTheDocument(); + + setDirectoryStatus(true); + rerender( {}} />); + expect(screen.queryByTestId('principal-picker-manual-hint')).toBeNull(); + }); +}); + +describe('PrincipalPicker — pill placement', () => { + it('renders badges below the input row in the DOM', () => { + setDirectoryStatus(false); + renderWithProviders( + {}} />, + ); + const input = screen.getByTestId('principal-picker-input'); + const badge = screen.getByTestId('principal-badge'); + // ``compareDocumentPosition`` returns bitmask DOCUMENT_POSITION_FOLLOWING (4) + // when the second node follows the first. The badge should follow the input. + expect(input.compareDocumentPosition(badge) & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy(); + }); +}); + +describe('PrincipalPicker — Browse dialog', () => { + beforeEach(() => { + setDirectoryStatus(true); + fetchMock.mockImplementation((url: RequestInfo | URL) => { + const u = url.toString(); + if (u.includes('/api/directory/search')) { + return Promise.resolve( + new Response( + JSON.stringify({ + results: [ + { type: 'group', id: 'Producers', display_name: 'Data Producers', sub_label: 'producers-guid' }, + ], + }), + { status: 200, headers: { 'Content-Type': 'application/json' } }, + ), + ); + } + return Promise.resolve(new Response('{}', { status: 200 })); + }); + }); + + it('clears the search query and filter when the dialog is reopened', async () => { + renderWithProviders( + {}} />, + ); + // Open once. + await userEvent.click(screen.getByRole('button', { name: /browse directory/i })); + const dialogInput1 = await screen.findByTestId('principal-picker-dialog-input'); + await userEvent.type(dialogInput1, 'pro'); + expect((dialogInput1 as HTMLInputElement).value).toBe('pro'); + + // Close via the in-dialog Close button (the path that previously + // bypassed reset because it called onOpenChange directly). Radix + // also renders an icon-only close button with sr-only "Close" text; + // pick the visible footer button (the one without the sr-only span). + const closeButtons = screen.getAllByRole('button', { name: /^close$/i }); + const visibleClose = closeButtons.find((b) => !b.querySelector('.sr-only'))!; + await userEvent.click(visibleClose); + await waitFor(() => { + expect(screen.queryByTestId('principal-picker-dialog-input')).toBeNull(); + }); + + // Reopen. + await userEvent.click(screen.getByRole('button', { name: /browse directory/i })); + const dialogInput2 = await screen.findByTestId('principal-picker-dialog-input'); + expect((dialogInput2 as HTMLInputElement).value).toBe(''); + }); + + it('picks a row on click and emits the principal id', async () => { + const onChange = vi.fn(); + renderWithProviders( + , + ); + await userEvent.click(screen.getByRole('button', { name: /browse directory/i })); + const dialogInput = await screen.findByTestId('principal-picker-dialog-input'); + await userEvent.type(dialogInput, 'pro'); + + const row = await screen.findByTestId('principal-row'); + await userEvent.click(row); + + expect(onChange).toHaveBeenLastCalledWith(['Producers']); + }); + + it('closes the dialog after a pick in multi-select mode', async () => { + // Browse is the discovery path -- it should always close after a + // pick so the user is returned to the form. Adding several entries + // is done via the inline type-ahead. + renderWithProviders( + {}} />, + ); + await userEvent.click(screen.getByRole('button', { name: /browse directory/i })); + const dialogInput = await screen.findByTestId('principal-picker-dialog-input'); + await userEvent.type(dialogInput, 'pro'); + + const row = await screen.findByTestId('principal-row'); + await userEvent.click(row); + + await waitFor(() => { + expect(screen.queryByTestId('principal-picker-dialog-input')).toBeNull(); + }); + }); + + it('row mousedown is default-prevented to keep focus on the input (regression for fix #1)', async () => { + renderWithProviders( + {}} />, + ); + await userEvent.click(screen.getByRole('button', { name: /browse directory/i })); + const dialogInput = await screen.findByTestId('principal-picker-dialog-input'); + await userEvent.type(dialogInput, 'pro'); + + const row = await screen.findByTestId('principal-row'); + const event = new MouseEvent('mousedown', { bubbles: true, cancelable: true }); + row.dispatchEvent(event); + expect(event.defaultPrevented).toBe(true); + }); +}); diff --git a/src/frontend/src/components/common/principal-picker.tsx b/src/frontend/src/components/common/principal-picker.tsx index 83828c66..8aeb695f 100644 --- a/src/frontend/src/components/common/principal-picker.tsx +++ b/src/frontend/src/components/common/principal-picker.tsx @@ -203,12 +203,6 @@ export function PrincipalPicker(props: PrincipalPickerProps) { return (

-
{configured ? ( )}
+ {!configured && ( + // Self-rendered hint so individual callers don't have to track + // configured vs. unconfigured state for their own labels. +

+ Press Enter, Tab, or comma to add an entry. +

+ )} + {configured && ( { addPrincipal(p); - if (!multiple) setDialogOpen(false); + // Always close after a pick -- even in multi-select mode. + // Browse is the discovery path; the inline type-ahead is the + // fast path for adding several entries in succession. + setDialogOpen(false); }} onSearchFail={markDegraded} /> @@ -395,6 +405,12 @@ function ConfiguredInput({ className="w-[var(--radix-popover-trigger-width)] p-1" align="start" onOpenAutoFocus={(e) => e.preventDefault()} + // Radix Dialog locks ``pointer-events: none`` on while open. + // The Popover is portaled to , so it inherits that lock and + // every row inside becomes unclickable when this picker is used + // inside a dialog (e.g. Role form, Team form, Owner dialog). + // Restoring auto here re-enables clicks for the popover subtree. + style={{ pointerEvents: 'auto' }} > {loading ? (
@@ -481,6 +497,10 @@ function PrincipalRow({ principal, onPick }: PrincipalRowProps) {