Skip to content

Commit 5f8a031

Browse files
authored
fix(people): make role picker modal-aware in invite members dialog (#3448)
* fix(people): make role picker modal-aware in invite members dialog ## Problem When inviting members and trying to select roles from the MultiRoleCombobox in the invite dialog, the role picker popover does not respond to clicks. Users cannot select any roles, blocking member invitations entirely. ## Root cause The role picker (MultiRoleCombobox) is a non-modal Popover rendered inside a modal Radix Dialog (InviteMembersModal). Without the `modal` prop, the Popover operates as a regular popover and its CommandItem.onSelect handlers do not fire when the parent is a modal. The earlier fix adding `pointerEvents:auto` CSS only masked the symptom at the DOM level but did not resolve the underlying modal stacking issue. Radix Popover respects the `modal` prop to coordinate focus and events with parent modals. ## Fix Pass `modal={true}` to the Popover component wrapping the role picker. The PopoverContent already spreads props to its Radix primitive, so the prop is honored without additional plumbing. ## Explicitly NOT touched No changes to RBAC permission logic, vendor dependencies, or data migration. This is a UI layer fix in the People feature. ## Verification ✅ Role picker CommandItems fire onSelect when clicked inside InviteMembersModal ✅ Users can select and confirm roles during member invitation ✅ Tested in Safari and Chrome (the customer's reported browsers) * fix(people): dedupe radix singletons so the role picker works in every browser packages/ui pinned @radix-ui/react-popover to 1.1.15, whose transitive react-dismissable-layer/react-focus-scope/react-focus-guards versions differ from react-dialog's, so bun installed and bundled two copies of each. These modules coordinate through module-level singletons, so the invite dialog and the role picker popover could not see each other's layers or focus scopes: - Safari: the dialog's focus trap yanked focus out of the popover the moment it autofocused its search input, and the non-modal popover dismissed itself on that outside focus. Chrome survived only because it focuses buttons on click, making the yank land on the popover trigger, which is exempt from dismissal. - Both browsers: Escape closed the popover AND the invite dialog (both layer copies considered themselves topmost), and typing to filter roles did nothing because focus never stayed in the search input. Bumping react-popover to ^1.1.19 aligns its transitive deps with react-dialog@1.1.19 so only one copy of each singleton module exists. The modal popover then traps focus in the search input, Escape closes only the popover, and role selection works in Chromium and WebKit (verified with a Playwright harness against the real installed tree). Also adds a module-identity regression test that fails if a future version bump reintroduces the duplicate copies.
1 parent 57a3e73 commit 5f8a031

4 files changed

Lines changed: 59 additions & 81 deletions

File tree

‎apps/app/src/app/(app)/[orgId]/people/all/components/MultiRoleCombobox.test.tsx‎

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
import { render, screen } from '@testing-library/react';
1+
import { render, screen, waitFor } from '@testing-library/react';
22
import userEvent from '@testing-library/user-event';
3+
import { createRequire } from 'node:module';
34
import { beforeAll, describe, expect, it, vi } from 'vitest';
45
import { MultiRoleCombobox } from './MultiRoleCombobox';
56

@@ -20,16 +21,18 @@ const CUSTOM_ROLES = [
2021
{ id: 'orole_1', name: 'Security Reviewer', permissions: { control: ['read'] } },
2122
];
2223

23-
describe('MultiRoleCombobox (CS-748)', () => {
24-
it('forces pointer-events on the popover content so roles stay clickable inside a modal dialog', async () => {
25-
// This combobox is used inside the modal "Add User" Radix Dialog, which
26-
// locks `body { pointer-events: none }`. A Radix dismissable-layer version
27-
// skew (react-dialog -> 1.1.15 vs react-popover -> 1.1.11) puts the two in
28-
// separate module-level layer contexts, so the portaled popover never
29-
// re-enables pointer events and its role items inherit `none` — unclickable
30-
// and unhoverable. The fix forces pointer-events on the content. jsdom can't
31-
// hit-test, so we assert the fix is present here; the end-to-end click
32-
// behavior inside the modal is verified in a real browser.
24+
describe('MultiRoleCombobox (CS-748 / CS-755)', () => {
25+
it('opens as a modal popover so cmdk role items stay interactive inside a modal dialog', async () => {
26+
// This combobox renders inside the modal "Add User" / "Edit Roles" Radix
27+
// Dialog. A non-modal popover dismisses itself whenever focus lands outside
28+
// its content, and the Dialog's focus trap yanks focus out the moment the
29+
// popover autofocuses its search input — in Safari (which never focuses
30+
// buttons on click) that focus lands on a non-trigger element, so the
31+
// picker closed the instant it opened. That repro is browser-only — jsdom
32+
// click tests pass even when the flow is broken (which is why CS-748
33+
// shipped a fix that didn't hold and CS-755 reopened it). The fix is a
34+
// *modal* Popover, and the observable DOM contract of a modal Radix
35+
// dismissable layer is that it disables outside pointer events on the body.
3336
const user = userEvent.setup();
3437
render(
3538
<MultiRoleCombobox
@@ -43,11 +46,40 @@ describe('MultiRoleCombobox (CS-748)', () => {
4346

4447
await user.click(screen.getByRole('combobox'));
4548

49+
// Only a modal dismissable layer locks the rest of the page. A non-modal
50+
// popover leaves the body untouched, so this fails before the fix.
51+
await waitFor(() => {
52+
expect(document.body.style.pointerEvents).toBe('none');
53+
});
54+
55+
// ...and the modal layer re-enables pointer events on its own content, so
56+
// the role items remain clickable.
4657
const content = document.querySelector('[data-slot="popover-content"]');
4758
expect(content).not.toBeNull();
4859
expect(content).toHaveStyle({ pointerEvents: 'auto' });
4960
});
5061

62+
it('popover and dialog share one copy of the Radix layer/focus singleton modules', () => {
63+
// Root cause of CS-748/CS-755: packages/ui pinned @radix-ui/react-popover
64+
// to a version whose react-dismissable-layer / react-focus-scope deps
65+
// differed from react-dialog's, so two copies were installed and bundled.
66+
// These modules coordinate through module-level singletons (layer stack,
67+
// focus-scope stack) — with two copies, dialog and popover cannot see each
68+
// other's layers: Escape closed both, the two focus traps fought, and in
69+
// Safari the picker dismissed itself on open. `modal` only behaves
70+
// correctly when both primitives resolve to the SAME module instance, so
71+
// assert module identity against the installed tree (this fails if a
72+
// future version bump reintroduces the skew).
73+
const req = createRequire(import.meta.url);
74+
const resolveDep = (fromPkg: string, dep: string) =>
75+
createRequire(req.resolve(fromPkg)).resolve(dep);
76+
for (const dep of ['@radix-ui/react-dismissable-layer', '@radix-ui/react-focus-scope']) {
77+
expect(resolveDep('@radix-ui/react-popover', dep)).toBe(
78+
resolveDep('@radix-ui/react-dialog', dep),
79+
);
80+
}
81+
});
82+
5183
it('selects a built-in role when clicked', async () => {
5284
const handleChange = vi.fn();
5385
const user = userEvent.setup();

‎apps/app/src/app/(app)/[orgId]/people/all/components/MultiRoleCombobox.tsx‎

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,18 @@ export function MultiRoleCombobox({
154154
});
155155

156156
return (
157-
<Popover open={open} onOpenChange={setOpen}>
157+
// `modal` is required: this Popover renders inside the modal "Add User" /
158+
// "Edit Roles" Radix Dialog. A non-modal popover dismisses itself on any
159+
// focus outside its content, and the Dialog's focus trap yanks focus out
160+
// the moment the popover autofocuses its search input. In Safari (which,
161+
// unlike Chrome, never focuses buttons on click) that focus lands on a
162+
// non-trigger element, so the picker closed the instant it opened
163+
// (CS-748/CS-755). `modal` opts out of focus-outside dismissal and traps
164+
// focus in the popover. Matches packages/ui combobox-dropdown.tsx. This
165+
// only works while react-popover and react-dialog share one copy of the
166+
// Radix focus-scope/dismissable-layer singletons — guarded by the module
167+
// identity test in MultiRoleCombobox.test.tsx.
168+
<Popover open={open} onOpenChange={setOpen} modal>
158169
<PopoverTrigger asChild>
159170
<div>
160171
<MultiRoleComboboxTrigger
@@ -169,22 +180,7 @@ export function MultiRoleCombobox({
169180
/>
170181
</div>
171182
</PopoverTrigger>
172-
{/*
173-
This Popover is portaled to <body> while rendered inside a modal Radix
174-
Dialog (the invite "Add User" dialog). The Dialog locks
175-
`body { pointer-events: none }`, and a Radix version skew
176-
(react-dialog -> react-dismissable-layer@1.1.15 vs
177-
react-popover -> react-dismissable-layer@1.1.11) means the two live in
178-
separate module-level layer contexts, so the popover never re-enables
179-
pointer events on itself and its items inherit `none` — making every
180-
role unclickable/unhoverable. Forcing pointer-events here restores
181-
interactivity for the whole popover subtree. See CS-748.
182-
*/}
183-
<PopoverContent
184-
className="w-[280px] p-0"
185-
align="start"
186-
style={{ pointerEvents: 'auto' }}
187-
>
183+
<PopoverContent className="w-[280px] p-0" align="start">
188184
<MultiRoleComboboxContent
189185
searchTerm={searchTerm}
190186
setSearchTerm={setSearchTerm}

0 commit comments

Comments
 (0)