fix(mosaic): coordinate pending identifier actions

.changeset/friendly-identifier-lock.md

@@ -0,0 +1,2 @@
+---
+---

packages/mosaic/docs/controllers.md

@@ -45,12 +45,39 @@ It owns the error (see "Errors"):
 const actions = usePendingAction({ errorFallback: messages.errors.generic });
 
 return {
-  rows: rows.map(row => (actions.errorKey === row.id ? { ...row, error: actions.error } : row)),
-  pendingId: actions.pendingKey,
+  rows: rows.map(row => ({
+    ...row,
+    isPending: actions.pendingKey === row.id,
+    error: actions.errorKey === row.id ? actions.error : undefined,
+  })),
   onRemove: id => void actions.run(id, () => onRemove(id)),
 };
 ```
 
+When direct actions and a confirmation must not run together, keep each owner
+and combine their pending state in the section controller:
+
+```ts
+const actions = usePendingAction();
+const remove = useConfirmController<UserProfileEmail>({
+  onConfirm: contact => removalFocus.remove(contact.id),
+  reverification,
+});
+
+return {
+  contacts: contacts.map(contact => ({ ...contact, isPrimaryPending: actions.pendingKey === contact.id })),
+  isDisabled: actions.isPending || remove.isPending,
+  remove,
+};
+```
+
+Pass `isDisabled` to every control that starts one of the actions, including the
+item that opens the confirmation. `remove.isPending` stays true after the dialog
+closes during a reverification until the confirmed action settles, and the
+controller ignores `confirm` until then, so closing the dialog never lets a
+second action start. Put each row's pending state on the row the controller
+returns, so views render it without comparing pending keys.
+
 `run` resolves `true` on success, so the caller can close a dialog or move
 focus. To hold pending through an OAuth redirect, await a delay inside the
 action (`user-profile-connected-accounts-section.controller.ts`). When the user
@@ -65,9 +92,9 @@ error and releases the lock. It is not for:
 
 `useConfirmController` earns a machine: the confirm is async, a failure must land
 back on the confirming step with a reason, and a cancelled reverification must
-close without one. `useUserButtonController` earns one because `open` and
-`pendingKey` constrain each other and dismissing must not abandon an in-flight
-invoke.
+close without one while the action it interrupted settles.
+`useUserButtonController` earns one because `open` and `pendingKey` constrain
+each other and dismissing must not abandon an in-flight invoke.
 
 `useConfirmController({ onConfirm, reverification, errorFallback })` owns one
 confirm dialog: open, pending, error, and the verify step a reverified action

packages/mosaic/src/blocks/confirm/__tests__/confirm.controller.test.ts

@@ -38,17 +38,18 @@ describe('confirmMachine', () => {
     expect(actor.getSnapshot().status).toBe('active');
   });
 
-  it('abandons the run when cancelled while pending, ignoring how it settles', async () => {
+  it('waits for a run cancelled while pending to settle, then returns to idle without its reason', async () => {
     const run = deferred<void>();
     const actor = start();
     actor.send({ type: 'CONFIRM', run: () => run.promise });
 
     actor.send({ type: 'CANCEL' });
-    expect(actor.getSnapshot().value).toBe('idle');
+    expect(actor.getSnapshot().value).toBe('cancelling');
+    actor.send({ type: 'OPEN' });
+    expect(actor.getSnapshot().value).toBe('cancelling');
 
     run.reject(blocked);
-    await run.promise.catch(() => undefined);
-    expect(actor.getSnapshot().value).toBe('idle');
+    await vi.waitFor(() => expect(actor.getSnapshot().value).toBe('idle'));
     expect(actor.getSnapshot().context.error).toBeUndefined();
   });
 
@@ -121,4 +122,44 @@ describe('useConfirmController', () => {
     });
     await waitFor(() => expect(result.current.isOpen).toBe(false));
   });
+
+  it.each(['resolve', 'reject'] as const)(
+    'stays pending after the dialog is cancelled until the run can %s, without a second run',
+    async outcome => {
+      const run = deferred<void>();
+      const onConfirm = vi.fn(() => run.promise);
+      const reverification = { status: 'loading' as const, onCancel: vi.fn(), visible: false };
+      const { result } = renderHook(() => useConfirmController({ onConfirm, reverification }));
+
+      act(() => result.current.onOpenChange(true));
+      act(() => result.current.confirm());
+      act(() => result.current.onOpenChange(false));
+
+      expect(reverification.onCancel).toHaveBeenCalledOnce();
+      expect(result.current.isOpen).toBe(false);
+      expect(result.current.isPending).toBe(true);
+
+      act(() => result.current.onOpenChange(true));
+      act(() => result.current.confirm());
+
+      expect(onConfirm).toHaveBeenCalledOnce();
+      expect(result.current.isOpen).toBe(false);
+
+      await act(async () => {
+        if (outcome === 'resolve') {
+          run.resolve();
+        } else {
+          run.reject(blocked);
+        }
+        await run.promise.catch(() => undefined);
+      });
+
+      expect(result.current.isPending).toBe(false);
+      expect(result.current.isOpen).toBe(false);
+      expect(result.current.errorMessage).toBeUndefined();
+
+      act(() => result.current.onOpenChange(true));
+      expect(result.current.isOpen).toBe(true);
+    },
+  );
 });

packages/mosaic/src/blocks/confirm/confirm.controller.ts

@@ -12,23 +12,23 @@ import { useMachine } from '../../machine/use-machine';
 import { toLocalizableError } from '../../utils/errors';
 
 export interface ConfirmContext {
-  run: () => Promise<void>;
+  run: Promise<void> | undefined;
   error: ErrorDescription | undefined;
 }
 
 export type ConfirmEvent = { type: 'OPEN' } | { type: 'CONFIRM'; run: () => Promise<void> } | { type: 'CANCEL' };
 
 const { createMachine, assign, fromPromise } = setup<ConfirmContext, ConfirmEvent>();
 
-function notSeated(): Promise<never> {
-  return Promise.reject(new Error('confirm run is not seated'));
+function seated(context: ConfirmContext): Promise<void> {
+  return context.run ?? Promise.reject(new Error('confirm run is not seated'));
 }
 
 export const confirmMachine = createMachine({
   id: 'confirm',
   initial: 'idle',
   context: {
-    run: notSeated,
+    run: undefined,
     error: undefined,
   },
   states: {
@@ -39,15 +39,15 @@ export const confirmMachine = createMachine({
     },
     confirming: {
       on: {
-        CONFIRM: { target: 'pending', actions: assign((_, event) => ({ run: event.run, error: undefined })) },
+        CONFIRM: { target: 'pending', actions: assign((_, event) => ({ run: event.run(), error: undefined })) },
         CANCEL: { target: 'idle', actions: assign(() => ({ error: undefined })) },
       },
     },
     pending: {
       on: {
-        CANCEL: { target: 'idle', actions: assign(() => ({ error: undefined })) },
+        CANCEL: { target: 'cancelling', actions: assign(() => ({ error: undefined })) },
       },
-      invoke: fromPromise(context => context.run(), {
+      invoke: fromPromise(seated, {
         onDone: { target: 'idle', actions: assign(() => ({ error: undefined })) },
         onError: [
           {
@@ -61,6 +61,9 @@ export const confirmMachine = createMachine({
         ],
       }),
     },
+    cancelling: {
+      invoke: fromPromise(seated, { onDone: 'idle', onError: 'idle' }),
+    },
   },
 });
 
@@ -112,12 +115,12 @@ export function useConfirmController<Payload = void>({
   const [snapshot, send] = useMachine(confirmMachine);
   const errorText = useErrorText();
   const { error } = snapshot.context;
-  const isPending = snapshot.value === 'pending';
-  const canClose = !isPending || Boolean(reverification?.onCancel);
+  const isPending = snapshot.value === 'pending' || snapshot.value === 'cancelling';
+  const canClose = snapshot.value !== 'pending' || Boolean(reverification?.onCancel);
 
   return {
     handle,
-    isOpen: snapshot.value !== 'idle',
+    isOpen: snapshot.value === 'confirming' || snapshot.value === 'pending',
     isPending,
     canClose,
     errorMessage: error ? errorText(error, errorFallback) : undefined,

packages/mosaic/src/blocks/confirmation/__tests__/confirmation.feature.test.tsx

@@ -82,6 +82,7 @@ describe('Confirmation', () => {
     await user.click(confirmButton());
     expect(onConfirm).toHaveBeenCalledOnce();
     expect(confirmButton()).toHaveAttribute('aria-busy', 'true');
+    expect(confirmButton()).toHaveFocus();
 
     run.resolve();
     await closed();

packages/mosaic/src/features/user-profile/user-profile-contact-list-row.view.tsx

@@ -10,7 +10,7 @@ import { Section } from '../../components/section';
 import { Plus } from '../../icons/glyphs/plus';
 import { fill, useMessages } from '../../localization';
 import { truncationStyles } from '../../styles/typography.styles';
-import type { UserProfileContact } from './user-profile-contact.types';
+import type { UserProfileContact, UserProfileContactRow } from './user-profile-contact.types';
 import { styles } from './user-profile-profile-panel.styles';
 
 export interface UserProfileContactListRowViewProps {
@@ -19,10 +19,10 @@ export interface UserProfileContactListRowViewProps {
   addAction?: ReactNode;
   kind: 'email' | 'phone';
   label: string;
-  items: UserProfileContact[];
+  items: UserProfileContactRow[];
   onAdd?: () => void;
   onVerify?: (id: string) => void;
-  pendingVerifyId?: string;
+  isDisabled?: boolean;
   onSetPrimary?: (id: string) => void;
   onRemove?: (item: UserProfileContact) => void;
   children?: ReactNode;
@@ -34,7 +34,7 @@ export function UserProfileContactListRowView({
   items,
   onAdd,
   onVerify,
-  pendingVerifyId,
+  isDisabled = false,
   onSetPrimary,
   onRemove,
   addAction,
@@ -65,6 +65,7 @@ export function UserProfileContactListRowView({
               size='sm'
               variant='outline'
               onClick={onAdd}
+              disabled={isDisabled}
             >
               <Icon
                 glyph={Plus}
@@ -92,10 +93,14 @@ export function UserProfileContactListRowView({
                 actions.push({
                   label: item.isDefault ? m.completeVerification : nouns.verify,
                   onClick: () => onVerify(item.id),
-                  status: pendingVerifyId === item.id ? 'pending' : 'idle',
+                  status: item.isVerifyPending ? 'pending' : 'idle',
                 });
               } else if (item.isVerified && !item.isDefault && onSetPrimary) {
-                actions.push({ label: m.setPrimary, onClick: () => onSetPrimary(item.id) });
+                actions.push({
+                  label: m.setPrimary,
+                  onClick: () => onSetPrimary(item.id),
+                  status: item.isPrimaryPending ? 'pending' : 'idle',
+                });
               }
 
               if (onRemove) {
@@ -120,6 +125,8 @@ export function UserProfileContactListRowView({
                       <ActionMenu
                         triggerRef={triggerRef?.(item.id)}
                         actions={actions}
+                        disabled={isDisabled}
+                        focusableWhenDisabled
                         label={fill(m.manageValue, { value: item.value })}
                       />
                     </Section.Actions>

packages/mosaic/src/features/user-profile/user-profile-contact.types.ts

@@ -5,6 +5,11 @@ export interface UserProfileContact {
   isVerified: boolean;
 }
 
+export interface UserProfileContactRow extends UserProfileContact {
+  isVerifyPending: boolean;
+  isPrimaryPending: boolean;
+}
+
 export type UserProfileEmail = UserProfileContact;
 
 export type UserProfilePhone = UserProfileContact;

packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-email-section.feature.test.tsx

@@ -72,6 +72,50 @@ async function enterCode(actor: Actor, code: string) {
 }
 
 describe('the user profile email section', () => {
+  it('holds the menu and row order while setting an address as primary, then returns focus', async () => {
+    const other = fapiEmailAddress({ id: 'idn_other', email_address: 'bob@example.com', verification: VERIFIED });
+    const { actor } = await renderSection(signedInWithEmails([PRIMARY, other]));
+    const primary = holdRequests('post', '/v1/me');
+    try {
+      await manageEmail(actor, 'bob@example.com', 'Set as primary');
+      await waitFor(() => expect(primary.requests).toHaveLength(1));
+      const menu = screen.getByRole('menu');
+      const item = within(menu).getByRole('menuitem', { name: 'Set as primary' });
+      expect(item).toHaveAttribute('aria-busy', 'true');
+      const manage = screen.getByRole('button', { name: 'Manage bob@example.com' });
+      expect(manage).toHaveAttribute('aria-disabled', 'true');
+      expect(within(menu).getByRole('menuitem', { name: 'Remove email' })).toHaveAttribute('aria-disabled', 'true');
+      expect(screen.queryByRole('alertdialog')).toBeNull();
+      expect(screen.getByRole('button', { name: 'Add email' })).toBeDisabled();
+      expect(await within(item).findByRole('progressbar', { name: 'Set as primary' })).toBeVisible();
+      expect(emailsListed()).toEqual(['alice@example.com', 'bob@example.com']);
+    } finally {
+      primary.release();
+      await waitFor(() =>
+        expect(within(emailRow()).getByText('bob@example.com').parentElement).toHaveTextContent('Primary'),
+      );
+    }
+    expect(screen.getByRole('button', { name: 'Manage bob@example.com' })).not.toHaveAttribute('aria-disabled', 'true');
+    await waitFor(() => expect(screen.queryByRole('menu')).toBeNull());
+    expect(emailsListed()).toEqual(['alice@example.com', 'bob@example.com']);
+    expect(screen.getByRole('button', { name: 'Manage bob@example.com' })).toHaveFocus();
+    expect(screen.queryByRole('progressbar', { name: 'Set as primary' })).toBeNull();
+  });
+
+  it('sets an address as primary once when Set as primary is double-clicked', async () => {
+    const other = fapiEmailAddress({ id: 'idn_other', email_address: 'bob@example.com', verification: VERIFIED });
+    const { actor, fapi } = await renderSection(signedInWithEmails([PRIMARY, other]));
+    const primary = holdRequests('post', '/v1/me');
+
+    await openMenu(actor, within(emailRow()).getByRole('button', { name: 'Manage bob@example.com' }));
+    await actor.dblClick(screen.getByRole('menuitem', { name: 'Set as primary' }));
+    await waitFor(() => expect(primary.requests).toHaveLength(1));
+
+    primary.release();
+    await waitFor(() => expect(fapi.client.sessions[0]?.user?.primary_email_address_id).toBe('idn_other'));
+    expect(primary.requests).toHaveLength(1);
+  });
+
   it('adds an address and verifies the code it was sent', async () => {
     const { actor } = await renderSection(signedInWithEmails([]));
     expect(emailRow()).toHaveTextContent('No email addresses added');
@@ -360,6 +404,7 @@ describe('email reverification', () => {
 
     await manageEmail(actor, 'bob@example.com', 'Set as primary');
     expect(await screen.findByText('Enter your current password to continue')).toBeVisible();
+    await waitFor(() => expect(screen.queryByRole('menu', { hidden: true })).toBeNull());
     await waitFor(() => expect(screen.getByLabelText('Password')).toHaveFocus());
     await actor.keyboard('hunter2{Enter}');
 
@@ -386,6 +431,7 @@ describe('email reverification', () => {
     expect(within(emailRow()).queryByRole('alert')).toBeNull();
     expect(within(emailRow()).getByRole('button', { name: 'Manage bob@example.com' })).toHaveFocus();
 
+    await waitFor(() => expect(screen.queryByRole('menu')).toBeNull());
     await manageEmail(actor, 'bob@example.com', 'Set as primary');
     await waitFor(() => expect(fapi.client.sessions[0]?.user?.primary_email_address_id).toBe('idn_second'));
   });
@@ -663,4 +709,29 @@ describe('Removing an address', () => {
     await waitFor(() => expect(emailsListed()).toEqual(['alice@example.com']));
     expect(screen.queryByRole('alertdialog')).toBeNull();
   });
+
+  it('locks the section while a removal runs, and unlocks it when the removal fails with the error in the dialog', async () => {
+    const second = fapiEmailAddress({ id: 'idn_second', email_address: 'bob@example.com', verification: VERIFIED });
+    const { actor } = await renderSection(signedInWithEmails([PRIMARY, second]));
+    const removal = holdRequests('post', '/v1/me/email_addresses/idn_second', { _method: 'DELETE' });
+    const manageAlice = () => screen.getByRole('button', { name: 'Manage alice@example.com', hidden: true });
+    const addEmail = () => screen.getByRole('button', { name: 'Add email', hidden: true });
+
+    await manageEmail(actor, 'bob@example.com', 'Remove email');
+    const dialog = await screen.findByRole('alertdialog', { name: 'Remove email address?' });
+    await actor.click(within(dialog).getByRole('button', { name: 'Remove' }));
+    await waitFor(() => expect(removal.requests).toHaveLength(1));
+
+    expect(manageAlice()).toHaveAttribute('aria-disabled', 'true');
+    expect(addEmail()).toBeDisabled();
+
+    removal.fail('last_identification', 'You cannot remove your last sign-in method.');
+    await waitFor(() =>
+      expect(within(dialog).getByRole('alert')).toHaveTextContent('You cannot remove your last sign-in method.'),
+    );
+    expect(dialog).toBeVisible();
+    expect(manageAlice()).not.toHaveAttribute('aria-disabled', 'true');
+    expect(addEmail()).toBeEnabled();
+    expect(emailsListed()).toEqual(['alice@example.com', 'bob@example.com']);
+  });
 });